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

Format files to prepare for pypi publish #10

Merged
merged 25 commits into from
Jul 9, 2020
Merged

Conversation

ajlee21
Copy link
Contributor

@ajlee21 ajlee21 commented Jul 1, 2020

This PR updates Readme and setup files to prepare for publishing this repo to pypi

@ajlee21 ajlee21 requested a review from dongbohu July 1, 2020 22:11
Copy link

@dongbohu dongbohu left a comment

Choose a reason for hiding this comment

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

Code looks good. Any idea why the two checks failed?

@ajlee21
Copy link
Contributor Author

ajlee21 commented Jul 2, 2020

The linter error is due to being on a fork: wearerequired/lint-action#13

The analysis error is because random is a requirement that is not found in pypi anymore. Its not called random2. I will change the code to use numpy random in this case.

@ajlee21
Copy link
Contributor Author

ajlee21 commented Jul 3, 2020

Based on discussion with @cgreene pulling out "extra" functions (i.e. functions specific to analysis performed in https://github.com/greenelab/simulate-expression-compendia) and leave only simulation functions (i.e dir setup, vae training, simulations).

Closing it for now while I add those changes and will reopen when its ready for review

@ajlee21 ajlee21 requested a review from ben-heil July 6, 2020 15:53
@ajlee21
Copy link
Contributor Author

ajlee21 commented Jul 6, 2020

This PR is now ready to review.

This PR complements: greenelab/simulate-expression-compendia#33

The ponyo repo included scripts to about the method to generate new simulated data as well as scripts specific to running the analysis found in this repo. This PR is removes those analysis-specific scripts.

The main changes in this PR are:

  1. Renaming and moving scripts in ponyo/
  2. Adding test notebooks to test each type of simulation approach human_tests
  3. Update Readme and setup.py files to get ready for pypi deployment.

@ajlee21 ajlee21 reopened this Jul 6, 2020
Copy link

@dongbohu dongbohu left a comment

Choose a reason for hiding this comment

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

Left some comments for you to consider.

.github/workflows/tests.yaml Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
- requests
Copy link

Choose a reason for hiding this comment

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

New line char needed.

human_tests/config_test_human.tsv Outdated Show resolved Hide resolved
]

# Check if analysis output directory exist otherwise create
for each_dir in output_dirs:
Copy link

Choose a reason for hiding this comment

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

Does it make more sense to merge this for loop with the one on lines 55-59?

ponyo/utils.py Outdated
os.makedirs(new_dir, exist_ok=True)

# Create results directories
output_dirs = [os.path.join(base_dir, dataset_name, "results")]
Copy link

Choose a reason for hiding this comment

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

This list includes only one element. The for loops on lines 65 and 71 seem redundant. If you do want to use for loop, these two loops can be merged into one.

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.

If the linter is never going to work on PRs, you may want to switch to something like this part of the actions I use in saged/whistl.
Though to use flake8 as a linter instead of an error checker, you would probably want the command to be

- name: Lint with flake8
      run: |
        $CONDA/bin/conda install flake8
        $CONDA/bin/flake8 . --count  --show-source --statistics

You can then add exceptions with --ignore as needed.

I approved the PR in case you want linter tinkering to be a different PR. Otherwise I can rereview it once the linter is working

.github/workflows/tests.yaml Outdated Show resolved Hide resolved
environment.yml Show resolved Hide resolved
ponyo/helper_vae.py Show resolved Hide resolved
ponyo/vae.py Outdated
@@ -106,7 +105,7 @@ def tybalt_2layer_model(
# The below is necessary for starting core Python generated random numbers
# in a well-defined state.

rn.seed(12345)
np.random.seed(12345)
Copy link
Contributor

Choose a reason for hiding this comment

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

You're setting the numpy seed twice in this file, and the seed for the base random package is no longer set. The second thing may not matter since you're not importing random in this file, but the first one appears to be a copy number variation

setup.cfg Outdated Show resolved Hide resolved
setup.py Show resolved Hide resolved
@ajlee21
Copy link
Contributor Author

ajlee21 commented Jul 9, 2020

Thank you for your comments!

@ben-heil about the linter, since the linter is working properly and its a permissions error with my fork I'll wait to see if this changes (since submitting feedback to them)

@ajlee21 ajlee21 merged commit 4ef7db8 into greenelab:master Jul 9, 2020
@ajlee21 ajlee21 deleted the pypi branch July 9, 2020 00:17
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.

4 participants