biocore / metagenomics_pooling_notebook

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

Added well_id_96 and tm10_8_tool as required columns #115

Closed charles-cowart closed 1 year ago

charles-cowart commented 1 year ago

This is the simplest update, where well_id_96 and tm10_8_tool are both required columns for pre-prep files.

charles-cowart commented 1 year ago

Hi @mmbryant23 @wasade ! Here is a PR where the two new columns are both required in the pre-prep file and appear in the output prep-info file. I just realized I should update the pre-prep file notebook as well. That update should appear shortly.

charles-cowart commented 1 year ago

@wasade @mmbryant23 After reviewing MacKenzie's original email, the well_id_96 column can be added to the plate_map file input of the notebook and pass through successfully without any modification to the notebook itself. The sample plate-map file still needs that column; MacKenzie if you don't mind, can you send me a range of sample values? I can use them to add meaningful examples in the sample plate-map file and finish the validation testing for them. Thanks!

charles-cowart commented 1 year ago

@mmbryant23 Reviewing the existing well-id code, is it safe to assume that valid values for well_id_96 will begin with a letter A through D and end with a number 1-10? e.g.: A1, A2, ...A10?

mmbryant23 commented 1 year ago

@charles-cowart - The well_id_96 column will consist of letter A through H, and end with numbers 1-12. I'll send over an example column in a few minutes. Thanks for checking!

charles-cowart commented 1 year ago

@mmbryant23 @wasade Ready for review!

charles-cowart commented 1 year ago

Actually I noticed a loss of precision on some floating points in one of the updated TSV files. Will let you know when additional fixes are ready.

charles-cowart commented 1 year ago

@wasade @mmbryant23 Thank you for your patience, it's now ready for review!

mmbryant23 commented 1 year ago

Thanks, Charlie! I think everything looks OK on my end. Checked all the expected values.

On Thu, Apr 20, 2023 at 8:57 PM Charles Cowart @.***> wrote:

@wasade https://github.com/wasade @mmbryant23 https://github.com/mmbryant23 Thank you for your patience, it's now ready for review!

— Reply to this email directly, view it on GitHub https://github.com/biocore/metagenomics_pooling_notebook/pull/115#issuecomment-1517108909, or unsubscribe https://github.com/notifications/unsubscribe-auth/AVY3ITUB4FUJF6MESL7MZ7DXCHLQ7ANCNFSM6AAAAAAXETKMEI . You are receiving this because you were mentioned.Message ID: @.***>

RodolfoSalido commented 1 year ago

the data in the well_id_96 column in the FinRisk test_data is completely wrong. How was that made?

charles-cowart commented 1 year ago

the data in the well_id_96 column in the FinRisk test_data is completely wrong. How was that made? @RodolfoSalido It was added by myself based on the parameters MacKenzie and Daniel gave. They're correct in that they conform to what was described above. I am more than happy to replace them with better values if you can provide them!

RodolfoSalido commented 1 year ago

I'm not sure if we need them to have accurate values, but right now multiple samples are linked to the same Well in the well_id_96 column. I was concerned that the wells were assigned programmatically incorrectly, but it doesn't sound like that is the case.

I stumbled into this because the new column requirement is causing some backward compatibility issues. I am updating the iSeqnorm test data to resolve the issue.

charles-cowart commented 1 year ago

I'm not sure if we need them to have accurate values, but right now multiple samples are linked to the same Well in the well_id_96 column. I was concerned that the wells were assigned programmatically incorrectly, but it doesn't sound like that is the case.

I stumbled into this because the new column requirement is causing some backward compatibility issues. I am updating the iSeqnorm test data to resolve the issue.

Let me know if I can help in some way!