-
Notifications
You must be signed in to change notification settings - Fork 4
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 qc section of recipe #72
Conversation
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 great to me. I made only minor comments that probably could use addressing before you merge. Thanks!
@@ -164,31 +166,30 @@ | |||
.reset_index() | |||
.rename(columns={0: "Ratio"}) | |||
) | |||
|
|||
empty_df=empty_df[['Percent_Empty']].stack().to_frame().reset_index().rename(columns={0: "Percent Empty"}) |
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 am not sure how else you are using empty_df
, but if possible, you should try to keep data processing of a single variable all in one spot. What I mean is move this line closer to where you define empty_df
(line 155).
This will help reader understanding a lot.
+ gg.coord_fixed() | ||
+ gg.geom_point(gg.aes(fill="Ratio"), shape='s', size=6) | ||
+ gg.geom_text(gg.aes(label="site_location"), size=6, color="lightgrey") | ||
+ gg.facet_wrap("~cell_quality_recode + well", ncol = 3, scales="fixed") |
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.
please make sure to run black
on this script.
(Black will make sure there are no spaces in ncol=3
😄 )
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.
hahaha. I can't even count the number of times I told myself "run black
before making the PR" and yet I still forgot!
+ gg.theme( | ||
axis_text=gg.element_blank(), | ||
axis_title=gg.element_blank(), | ||
strip_background=gg.element_rect(colour="black", fill="#fdfff4"), | ||
strip_background=gg.element_rect(colour="black", fill="#fdfff4", height = 1/5), |
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.
black will fix this too
@@ -291,42 +347,40 @@ | |||
# Any point too high or too low may have focus issues | |||
pll_col_prefix = "ImageQuality_PowerLogLogSlope_" | |||
PLLS_df_cols = image_meta_col_list.copy() | |||
PLLS_cols = [] | |||
for name in painting_image_names: |
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.
Just confirming that you no longer create a figure for all painting_image_names
. Instead you are creating a single figure, correct?
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 don't think painting_image_names
was ever used to make separate figures? Yes, this is a single figure for all the painting images.
@gwaygenomics Did a little more cleanup.
|
very nice - please merge when you are ready. (I assume you have write access) |
Updates QC section including:
Major changes:
Minor changes: