biocore / metagenomics_pooling_notebook

Jupyter notebooks to assist with sample processing
MIT License
8 stars 16 forks source link

Amplicon prep fix #56

Closed charles-cowart closed 2 years ago

charles-cowart commented 2 years ago

Fixed undetected merge conflict in a sample output file. Added eight new blank columns and reordered them as requested by the wet-lab. Sample output file added to test_output for reference.

charles-cowart commented 2 years ago

test_output/amplicon/2021_08_17_THDMI-4-6_samplesheet.csv contains a merge conflict that was never resolved. This was probably overlooked because there aren't unittests associated with the iPython notebooks. Also, since there are a large number of changes, the diff wouldn’t appear in GitHub during a code review by default.

Resolved the merge conflict, preferring rows in [Data] with I7_Index_ID values like ‘515rcbc0’ over ‘EukBr_Hiseq_0017’. Observed that the indexes also changed for each sample. For example, ‘X00180471’ now has index ‘AGCCTTCGTCGC’ instead of ‘ACGAGACTGATT’. I believe this is normal behavior. Can @ahdilmore or @ElDeveloper please confirm? Thanks! Observed that values for I5_Index_ID, index2, and Well_description are also empty.

charles-cowart commented 2 years ago

Since Amplicon prep-files are currently generated by the Ipython notebook, I modified it to reorder the columns as mentioned in https://github.com/biocore/metagenomics_pooling_notebook/issues/54#issue-1069067398. As requested by the wet-lab, I've introduced the new columns with blank values that they will fill in as needed.

Ultimately this would be good to resolve as discussed in https://github.com/biocore/metagenomics_pooling_notebook/issues/40

charles-cowart commented 2 years ago

Adding @antgonza just to keep him in the loop. It doesn't warrant more than one person to review, IMHO.

charles-cowart commented 2 years ago

No unittests were added since we don't appear to be doing that for the notebooks. IMHO it would be good to be testing this functionality in the future.

ahdilmore commented 2 years ago

Resolved the merge conflict, preferring rows in [Data] with I7_Index_ID values like ‘515rcbc0’ over ‘EukBr_Hiseq_0017’. Observed that the indexes also changed for each sample. For example, ‘X00180471’ now has index ‘AGCCTTCGTCGC’ instead of ‘ACGAGACTGATT’. I believe this is normal behavior. Can @ahdilmore or @ElDeveloper please confirm? Thanks! Observed that values for I5_Index_ID, index2, and Well_description are also empty.

I think this is expected, but it might be best to confirm with the wet lab. Thank you for getting this done, Charlie!

mmbryant23 commented 2 years ago

not sure if replying to this or directly works better (sorry) -

AGCCTTCGTCGC should be the correct barcode for X00180471 for 16s - we'll keep an eye on this when we test

On Mon, Dec 6, 2021 at 9:57 AM Hazel Dilmore @.***> wrote:

Resolved the merge conflict, preferring rows in [Data] with I7_Index_ID values like ‘515rcbc0’ over ‘EukBr_Hiseq_0017’. Observed that the indexes also changed for each sample. For example, ‘X00180471’ now has index ‘AGCCTTCGTCGC’ instead of ‘ACGAGACTGATT’. I believe this is normal behavior. Can @ahdilmore https://github.com/ahdilmore or @ElDeveloper https://github.com/ElDeveloper please confirm? Thanks! Observed that values for I5_Index_ID, index2, and Well_description are also empty.

I think this is expected, but it might be best to confirm with the wet lab. Thank you for getting this done, Charlie!

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/biocore/metagenomics_pooling_notebook/pull/56#issuecomment-987016317, or unsubscribe https://github.com/notifications/unsubscribe-auth/AVY3ITTHMOA3HDFXD4N75QLUPT2QZANCNFSM5JNKVLPA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

mmbryant23 commented 2 years ago

Thanks MacKenzie!

From: MacKenzie Bryant @.> Date: Monday, December 6, 2021 at 10:08 AM To: biocore/metagenomics_pooling_notebook @.>, Amanda H Dilmore @.>, Charles Cowart @.> Cc: biocore/metagenomics_pooling_notebook @.>, Subscribed @.> Subject: Re: [biocore/metagenomics_pooling_notebook] Amplicon prep fix (PR #56) not sure if replying to this or directly works better (sorry) -

AGCCTTCGTCGC should be the correct barcode for X00180471 for 16s - we'll keep an eye on this when we test

On Mon, Dec 6, 2021 at 9:57 AM Hazel Dilmore @.**@.>> wrote:

Resolved the merge conflict, preferring rows in [Data] with I7_Index_ID values like ‘515rcbc0’ over ‘EukBr_Hiseq_0017’. Observed that the indexes also changed for each sample. For example, ‘X00180471’ now has index ‘AGCCTTCGTCGC’ instead of ‘ACGAGACTGATT’. I believe this is normal behavior. Can @ahdilmorehttps://urldefense.com/v3/__https:/github.com/ahdilmore__;!!LLK065n_VXAQ!3rG-VeuL1UQmgyx3Ebo-mSSjfeVMHzlHBXwtklyWXw9v0DxAMh-U08nBnAZz4KkvOA$ or @ElDeveloperhttps://urldefense.com/v3/__https:/github.com/ElDeveloper__;!!LLK065n_VXAQ!3rG-VeuL1UQmgyx3Ebo-mSSjfeVMHzlHBXwtklyWXw9v0DxAMh-U08nBnAYUsMvdJA$ please confirm? Thanks! Observed that values for I5_Index_ID, index2, and Well_description are also empty.

I think this is expected, but it might be best to confirm with the wet lab. Thank you for getting this done, Charlie!

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHubhttps://urldefense.com/v3/__https:/github.com/biocore/metagenomics_pooling_notebook/pull/56*issuecomment-987016317__;Iw!!LLK065n_VXAQ!3rG-VeuL1UQmgyx3Ebo-mSSjfeVMHzlHBXwtklyWXw9v0DxAMh-U08nBnAbzQr1kuA$, or unsubscribehttps://urldefense.com/v3/__https:/github.com/notifications/unsubscribe-auth/AVY3ITTHMOA3HDFXD4N75QLUPT2QZANCNFSM5JNKVLPA__;!!LLK065n_VXAQ!3rG-VeuL1UQmgyx3Ebo-mSSjfeVMHzlHBXwtklyWXw9v0DxAMh-U08nBnAaKiGr8oA$. Triage notifications on the go with GitHub Mobile for iOShttps://urldefense.com/v3/__https:/apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675__;!!LLK065n_VXAQ!3rG-VeuL1UQmgyx3Ebo-mSSjfeVMHzlHBXwtklyWXw9v0DxAMh-U08nBnAYUAlgh8g$ or Androidhttps://urldefense.com/v3/__https:/play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign*3Dnotification-email*26utm_medium*3Demail*26utm_source*3Dgithub__;JSUlJSU!!LLK065n_VXAQ!3rG-VeuL1UQmgyx3Ebo-mSSjfeVMHzlHBXwtklyWXw9v0DxAMh-U08nBnAb8shKcKg$.

charles-cowart commented 2 years ago

Resolved the merge conflict, preferring rows in [Data] with I7_Index_ID values like ‘515rcbc0’ over ‘EukBr_Hiseq_0017’. Observed that the indexes also changed for each sample. For example, ‘X00180471’ now has index ‘AGCCTTCGTCGC’ instead of ‘ACGAGACTGATT’. I believe this is normal behavior. Can @ahdilmore or @ElDeveloper please confirm? Thanks! Observed that values for I5_Index_ID, index2, and Well_description are also empty.

I think this is expected, but it might be best to confirm with the wet lab. Thank you for getting this done, Charlie!

My pleasure! Thank you guys for confirming!

charles-cowart commented 2 years ago

@ElDeveloper Thanks for your patience! I've migrated the new code into metapool and updated the tests.

ElDeveloper commented 2 years ago

Thans @charles-cowart