Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add multiple enrichment methods #49

Merged
merged 1 commit into from
Dec 18, 2020
Merged

Conversation

ajlee21
Copy link
Contributor

@ajlee21 ajlee21 commented Dec 8, 2020

The goal of this PR was to add other multiple enrichment methods to demonstrate the our simulation method can be plugged into other pipelines easily.

The changes to this PR will depend on changes in this PR #50. So for now I want to merge these changes in the in term and I'll open a new PR in the future to finish the work that this PR started.

The main change in this PR can be found in Try_other_enrichment_methods.ipynb. There is not much in this notebook. It mainly starts to outline and setup the changes to be made. This doesn't need a heavy review as this will be changed in the future.

@ajlee21 ajlee21 requested a review from danich1 December 17, 2020 17:54
@ajlee21 ajlee21 marked this pull request as ready for review December 17, 2020 17:54
Copy link
Contributor

@danich1 danich1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Left tiny comments for over grand-scheme, but no changes necessary on this PR.

from rpy2.robjects import pandas2ri
pandas2ri.activate()

from ponyo import utils
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What tensorflow version are you using? If it is less than 2.x.x, your ponyo code might become outdated. (Example Here Really tiny issue in the overall scheme, but I wanted to just let you know.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for looking out. I remember having some conflicts with other dependencies that arose when i previously tried to upgrade tensorflow. But it has been a while since I looked at this so I'll make an issue in my ponyo repo, which is using v<2.x.x for tensorflow



# Read in config variables
base_dir = os.path.abspath(os.path.join(os.getcwd(), "../"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might want to try and use pathlib. I know broken record, but it definitely makes os path generation a lot easier to deal with.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a broken record. I definitely agree with using paths instead of the way I currently have my config. The legacy reason for having the config this way is 1) to be consistent with how ponyo is, which this repo uses and 2) it seemed more user friendly at the time to have a tab-delimited file and have that passed into the notebook as opposed to "importing a python file. I also wasn't aware, at the time when I created the config, that I could define both variables and paths in the same file. But rest assured that my other newer repo uses paths:)

@ajlee21 ajlee21 merged commit c9db405 into greenelab:master Dec 18, 2020
@ajlee21 ajlee21 deleted the other_enrich branch December 18, 2020 20:44
@ajlee21 ajlee21 mentioned this pull request Jan 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants