-
Notifications
You must be signed in to change notification settings - Fork 26
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
Temporarily remove pixel preprocessing normalization for Candace's paper #913
Conversation
@cliu72 is it fine to leave in the computations of |
… temp_norm_purge
…g to MapDataToNodes
I think it's okay to leave in the computations to make it easier for later, but can we remove writing those files? The files with the 99.9% value and the pixel threshold value. I think having those files written will be confusing. |
@cliu72 we're aiming to get this PR merged in by Friday so we can close out as many open Pixie issues as possible before your submission. |
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!
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.
Lets wait to merge this in until all of the other PRs are merged in, as well as after all the testing candace does, to make sure that undoing this is as simple as possible.
@ngreenwald @cliu72 with the overwrite branch now merged into |
I'm going to do one more check over everything, which I'll do hopefully today/tomorrow. I think we can wait to merge this PR then after that. |
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.
Merge conflicts with the refactoring branch. Once those are resolved this is good to go.
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.
Not sure what happened here, but none of the changes are present anymore
Oh that's weird, let me fix that up again. |
Looks like GitHub had trouble mapping the changes into pixie_preprocessing.py. Should be good to go now! |
I don't think all the changes have been re-implemented after the merge conflicts. These lines should be gone (were removed in the original review): ark-analysis/src/ark/phenotyping/pixie_preprocessing.py Lines 68 to 70 in 8efc712
ark-analysis/src/ark/phenotyping/pixie_preprocessing.py Lines 152 to 157 in 8efc712
After thinking on it some more, I think it would be best to remove these too (to avoid confusion): ark-analysis/src/ark/phenotyping/pixie_preprocessing.py Lines 258 to 284 in 8efc712
ark-analysis/src/ark/phenotyping/pixie_preprocessing.py Lines 322 to 347 in 8efc712
Remove |
@cliu72 OK this should do the trick. |
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!
What is the purpose of this PR?
@cliu72 will need to remove
channel_norm_df
normalization andpixel_thresh
from the pixel preprocessing step of Pixie.How did you implement your changes
Explicitly remove both of these processes.
Remaining issues
See discussion thread.