-
Notifications
You must be signed in to change notification settings - Fork 3
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
Update ponyo simulations #20
Conversation
Pull Request Test Coverage Report for Build 394793710
💛 - Coveralls |
There was a problem hiding this 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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Just a few comments.
""" | ||
Returns sample ids (found in gene expression df) associated with | ||
a given list of experiment ids (found in the metadata) | ||
|
||
Arguments | ||
---------- | ||
experiment_ids_file: str | ||
File containing all cleaned experiment ids | ||
metadata_filename: str |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be useful to link to an example of a metadata file here (e.g. the one in human_tests).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good tip
ponyo/simulate_expression_data.py
Outdated
@@ -437,20 +469,14 @@ def shift_template_experiment( | |||
|
|||
Arguments | |||
---------- | |||
normalized_data_file: str | |||
File containing normalized gene expression data | |||
normalized_data: str |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be a pd.DataFrame
, right? Not a str
?
@@ -199,12 +199,15 @@ def simulate_by_random_sampling( | |||
|
|||
def simulate_by_latent_transformation( | |||
num_simulated_experiments, | |||
normalized_data_file, | |||
normalized_data_filename, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing you could think about is changing these simulate_by_X
functions to also take a DataFrame as input (i.e. normalized_data
rather than normalized_data_filename
). This would make them consistent with shift_template_experiment
, as well as possibly making them faster (although I'm not sure if you're calling these ones in a loop anywhere).
Might be a non-trivial amount of work to update other projects to conform with this, though, so if you want to create a separate issue and think about it later that's fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! I was waffling about this too. So the other two methods don't run in a for loop so its not required that we use the dataframe. I left them because it makes more sense me to input filenames instead of dataframes (i know its an extra step but feels like its one less hassle) and this one method is more of an exception.
I'll add an issue. One thing I could see doing is making a wrapper function to call the for loop
This PR addresses issues #18 and #17
This PR also updates the test scripts based on these new changes.
Some minor associated updates:
This will be ponyo v 0.3
Note: Linter is failing due to being on a fork