-
Notifications
You must be signed in to change notification settings - Fork 5
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
Compare feature space with Mitocheck #13
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
@@ -0,0 +1,696 @@ | |||
{ |
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.
Line #25. if jump_random_sample_n > 0:
Consider throwing an error if this condition isn't satisfied
Reply via ReviewNB
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.
Thank you for this suggestion! It may not be best practice, but given this function is defined locally to the notebook and is not expected to be used outside, it is self-contained and not meant for additional usage.
I believe that improving the function in this scenario in this specific way (to account for potential user error) is not a high value exercise. I probably, however, should do this more often when actively developing!
@@ -0,0 +1,696 @@ | |||
{ |
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.
Line #39. results = stats.kstest(mitocheck_distribution, jump_distribution)
The defaults should work here
Reply via ReviewNB
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.
Thanks for checking!
@@ -0,0 +1,696 @@ | |||
{ |
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.
@@ -0,0 +1,696 @@ | |||
{ |
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.
Line #9. for feature in all_features:
Could also use list or dictionary comprehension
Reply via ReviewNB
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.
I agree! FWIW, I opted to use a regular loop to increase readability. Sometimes I get confused with list comprehension, which decreases understandability.
@@ -0,0 +1,696 @@ | |||
{ |
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.
Line #12. result = compare_dataset_features(
May also consider introducing some test correction such as Bonferroni.
Reply via ReviewNB
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.
Thanks for this note! I think that we will correct for tests downstream, when we report results and visualize. Definitely important to consider!
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.
LGTM @gwaybio! I like the reproducibility considerations!
Thanks for the review @MattsonCam ! I made one minor adjustment (fixed typo) and will now merge. |
KS test analysis comparing 1,000 permutations of randomly sampled JUMP cells from a single plate and Mitocheck cells.
We perform the KS test per CellProfiler feature in common. The goal is to identify the differences in feature distributions between JUMP and mitocheck