-
Notifications
You must be signed in to change notification settings - Fork 72
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
Small fixes to sample QC notebook #773
Conversation
scripts/notebooks/SampleQC.ipynb
Outdated
"sample_set_tbl = sample_set_tbl[sample_set_tbl['entity:sample_set_id'].str.contains('KJ_EvidenceQC_Updates')]" | ||
"sample_set_tbl = sample_set_tbl[sample_set_tbl['entity:sample_set_id'].str.contains('KJ_EvidenceQC_Updates')]\n", | ||
"file_path = generate_file_path(TLD_PATH, 'artifacts', 'sample_sets_qc.tsv')\n", | ||
"save_df(WS_BUCKET, file_path, sample_set_tbl)" |
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.
Can we comment out the reference to KJ_EvidenceQC_Updates
? I see it's used as an example, but probably should be commented out. And maybe even change the substring being searched for to something more generalizable rather than 'KJ'.
This must've been missed in the first pass.
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.
Maybe you can add 1-2 new lines between the comment to optionally filter to only include a subset of batches. and the two lines that now follow it (generate_file_path
and save_df
)?
I worry that folks will simply ignore a single-line comment. Alternatively, we can add a green box for (optional) user input to make this more clear, but believe we decided against this in previous iterations.
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.
Could add a green box and a new variable SUBSTRING and only run the following lines if SUBSTRING is not None?
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.
Sounds 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.
Looks good to me, thanks for these cleanups - just one minor comment about the formatting relating to subsetting the batches to process.
Looks great now, thanks! |
Updates include:
Testing:
Tested in clone of public workspace on reference panel data, with and without reference PED file. Cleared outputs and tested re-upload