biocore / metagenomics_pooling_notebook

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

iSeqnorm pooling notebook. #89

Closed RodolfoSalido closed 1 year ago

RodolfoSalido commented 1 year ago

I made a new pooling notebook for the iSeqnorm pipeline. I added new visualizations and wrote 4 functions that could be merged into metapool.py: calculate_iseqnorm_pooling_volumes() linear_transform() read_survival() merge_read_counts()

Unfortunately, I haven't written any tests for them.

Let me know if you need anything from me.

charles-cowart commented 1 year ago

@RodolfoSalido The number of files to review jumped to 27. I'm not sure if this is because I inadvertently merged master into it in order to manually resolve a conflict. Looking into it.

charles-cowart commented 1 year ago

Hi @RodolfoSalido in test_metapool.py, there are several lines calling the join() function instead of os.path.join() or defining 'from os.path import join' at the top of the file. Can you please change all occurrences to use either join() or os.path.join()? Please note that there are also a few lines that are calling string.join() which is a different function entirely so you'll want to be careful with search and replace. This should get most of the tests to pass.

charles-cowart commented 1 year ago

Hi @RodolfoSalido it looks like there are four tests failing and one test has an error. It looks like at least some of them such as test_estimate_read_depth(), the observed results from calculate_iseqnorm_pooling_volumes() differ from the expected values of the TSV file by a single row or column. Can you please confirm that the following tests work as intended? test_estimate_read_depth test_find_threshold_autopool test_merge_read_counts test_read_pico_csv test_read_plate_map_csv_validate_qiita_sample_names

You can review the error messages here: https://github.com/biocore/metagenomics_pooling_notebook/actions/runs/4611263674/jobs/8150742325?pr=89

charles-cowart commented 1 year ago

@RodolfoSalido it looks like we are down to one error and two failures. I'll pull the latest and see if I can't resolve these. If I have any questions I'll ping you. Thanks!

charles-cowart commented 1 year ago

@RodolfoSalido It looks like earlier today you pushed a commit with only a few file changes, followed by a second commit later that added most of the 27 new files that we're seeing. Can you confirm that this is correct? For the record I am usually in favor of adding as many sample input/output files as we need.

charles-cowart commented 1 year ago

@RodolfoSalido I pushed a fix for a single test to confirm that I could push to your repo. I'll be pushing fixes for the remaining tests as soon as possible.

RodolfoSalido commented 1 year ago

Yes, should be correct. The commit with the 27ish file changes included the unit_tests and a bunch of input and output files.

-Rodolfo

On Apr 4, 2023, at 1:55 PM, Charles Cowart @.***> wrote:

@RodolfoSalido https://github.com/RodolfoSalido It looks like earlier today you pushed a commit with only a few file changes, followed by a second commit later that added most of the 27 new files that we're seeing. Can you confirm that this is correct? For the record I am usually in favor of adding as many sample input/output files as we need.

— Reply to this email directly, view it on GitHub https://github.com/biocore/metagenomics_pooling_notebook/pull/89#issuecomment-1496595183, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADEVFIY5YSQLSZE5YCLSI7TW7SDFXANCNFSM6AAAAAAU34LZUU. You are receiving this because you were mentioned.

charles-cowart commented 1 year ago

Yes, should be correct. The commit with the 27ish file changes included the unit_tests and a bunch of input and output files.

Great, thanks Rodolfo!

charles-cowart commented 1 year ago

@RodolfoSalido All tests are passing! I will approve and merge.