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

Pip refactor #31

Merged
merged 9 commits into from Jun 15, 2020
Merged

Pip refactor #31

merged 9 commits into from Jun 15, 2020

Conversation

ajlee21
Copy link
Contributor

@ajlee21 ajlee21 commented Jun 15, 2020

This PR updates the notebooks to import ponyo modules (https://github.com/ajlee21/ponyo), which contains all the functions from this repository and modularizes them.

This PR also updates the environment file to automatically install R packages.

All notebooks were re-run on a small test dataset to make sure everything runs as expected. Test datasets and outputs are not seen in this PR.

@ajlee21 ajlee21 requested review from danich1 and ben-heil June 15, 2020 13:30
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!

Copy link
Contributor

@ben-heil ben-heil left a comment

Choose a reason for hiding this comment

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

This looks like a lot of comments, but is really only two different points. One is that since your functions are coming from ponyo you can remove a bunch of pythonpath tinkering.

The second is that you don't currently have ponyo in your environment file so this PR breaks your code

Human/nbconverted/Human_experiment_lvl_sim.py Outdated Show resolved Hide resolved
@@ -1,4 +1,4 @@

#!/usr/bin/env python
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't a criticism so much as something I'm curious about: why did you decide to add a shebang line here? I don't see it very frequently for python scripts, though I imagine there's a reason for you adding it

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it something that nbconvert started doing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I hadn't noticed that before. It does look like its something that is added when you nbconvert: jupyter/nbconvert#690

Human/nbconverted/Human_sample_lvl_sim.py Outdated Show resolved Hide resolved
Pseudo_experiments/nbconverted/create_heatmap.py Outdated Show resolved Hide resolved
Pseudomonas/nbconverted/Pseudomonas_experiment_lvl_sim.py Outdated Show resolved Hide resolved
Pseudomonas/nbconverted/Pseudomonas_sample_lvl_sim.py Outdated Show resolved Hide resolved
- conda-forge::libiconv=1.15
- conda-forge::joblib=0.13.2
- pip=9.0
- anaconda::protobuf
- pip=19.2.1
- pip:
- torch>=0.4.0
- matplotlib==3.0.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't forget to add ponyo to your environment file. You could do a pip install pointing to a github link for now, then update it once ponyo is added to pypi

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently I am manually pip installing ponyo into this conda environment because ponyo is not uploaded to pypi just yet. I plan to add ponyo to this yml file once it is.

@ajlee21
Copy link
Contributor Author

ajlee21 commented Jun 15, 2020

Let me know if the changes look ok

@ajlee21 ajlee21 requested a review from ben-heil June 15, 2020 20:36
Copy link
Contributor

@ben-heil ben-heil left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@ajlee21 ajlee21 merged commit 8dc2aea into greenelab:master Jun 15, 2020
@ajlee21 ajlee21 deleted the pip_refactor branch June 15, 2020 21:42
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.

None yet

3 participants