search

biocore / metagenomics_pooling_notebook

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

Updates to Dev branch #161

Closed charles-cowart closed 6 months ago

charles-cowart commented 7 months ago

Updates based on review of recent changes.

charles-cowart commented 7 months ago

Hi @RodolfoSalido ! I was reviewing the differences between dev and master when I decided to review the notebooks and files that touch 2017-08-04_MRSA_1-4_sample_sheet.csv in test_output.

I made a change to what validate_sample_sheet() returns for clarity, now that it's a method of an class instead of a global function. I updated the csv file, as well as the two notebooks that touch it. Metagenomics_samplesheet_generator notebook looks good, but I encountered an error when running Metagenomics_pipeline_MiniPiconorm. The changes I made to it should work, but I can't resolve the error in one of the inputs to make_sample_sheet(). Would you mind checking this out as well?

You'll want to pull down a fresh copy of my dev branch most likely, in order to get these mods and debug MiniPiconorm notebook. If you want to push to my dev branch or merge this first into dev and do your mod separately, either way is fine with me.

charles-cowart commented 7 months ago

hi @RodolfoSalido! I noticed that this test has been commented out entirely. Do you know if we still need it?

https://github.com/charles-cowart/metagenomics_pooling_notebook/blob/c5654b219df70ca01fbb2542946d0f2d0e7eba74/metapool/tests/test_metapool.py#L92-L123

RodolfoSalido commented 7 months ago

I honestly think that the Metagenomics_pipeline_MiniPiconorm notebook can be deprecated. It practically got replaced by the iSeqnorm notebook, and all of the functions in the MiniPiconorm notebook are present in the iSeqnorm notebook. The switch from fluorescent quantification to iSeq sequence counts for pool normalization triggered the creation of the iSeqnorm notebook and the deprecation of the MiniPiconorm notebook.

RodolfoSalido commented 7 months ago

The commented out test is for a function that is no longer used by the notebooks. The function tested was replaced by autopool(), which in turn is also used with a very limited scope nowadays.

I think we an get rid of the test and that won't affect the codebase that is actually being used by the wetlab.

charles-cowart commented 6 months ago

I honestly think that the Metagenomics_pipeline_MiniPiconorm notebook can be deprecated. It practically got replaced by the iSeqnorm notebook, and all of the functions in the MiniPiconorm notebook are present in the iSeqnorm notebook. The switch from fluorescent quantification to iSeq sequence counts for pool normalization triggered the creation of the iSeqnorm notebook and the deprecation of the MiniPiconorm notebook.

I think that's a good point. Since @mmbryant23 is out, I'm going to take the liberty of removing it and all files associated with only it. Otherwise, the next time we revisit this issue could be after you've graduated.