search

biocore / metagenomics_pooling_notebook

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

Notebook reorganization #62

Closed kfogelso closed 2 years ago

kfogelso commented 2 years ago

Addressing issues #24 #25

I also documented error messages (mostly minor) I came across while running through the various notebooks, which we can go over and assign at a later date.

@ahdilmore @cguccione @antgonza

antgonza commented 2 years ago

@kfogelso, if I understand this correctly, the goal of this PR is to move files around and add testing data. The first is easy to see but I'm a bit confused about the second: it seems like there is not huge difference between the action running time between your PR and previous ones, which to me it means there are no new tests being run with the test data. However, it seems like there is a slight increment in coverage; could you clarify this?

Thank you.

kfogelso commented 2 years ago

@antgonza from what I understand, for #25 @ElDeveloper wanted me to run cell by cell through every notebook and document if/where I'm getting errors, as well as if/where there are test data/test outputs missing. Since last running through the notebooks, it looks like some of this issues I was seeing previously have been resolved. However, I identified one needed test_data file not on the repo (in metagenomics_repooling notebook ./finrisk_data/complete_dataframes/FinRisk_5-8_back-compiled_dummy.txt). Was planning on talking through minor error messages w/ @cguccione and @ahdilmore at a later point to see if it these are things that can be quickly addressed.

Does this answer your question?

antgonza commented 2 years ago

It does, thank you for the explanation.

antgonza commented 2 years ago

Forgot to add, then I think it seems fine but I think it will be better if we wait for @ElDeveloper feedback.

antgonza commented 2 years ago

@ElDeveloper, friendly ping about this.

antgonza commented 2 years ago

Thank you everyone. Just for the record: @kfogelso and I met and addressed @ElDeveloper comments so I think is fine to merge.