search

biocore / metagenomics_pooling_notebook

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

Fix negative plate reader concentration handling #58

Closed RodolfoSalido closed 2 years ago

RodolfoSalido commented 2 years ago

Fixed read_pico_csv() to limit concentration range to (0 , 60)

antgonza commented 2 years ago

Hey @RodolfoSalido, you need to solve conflicts for this to move forward. If you do git pull https://github.com/biocore/metagenomics_pooling_notebook master it should download the latests version of master and show you the conflicts so you can fix them.

RodolfoSalido commented 2 years ago

I think I resolved the conflict?

antgonza commented 2 years ago

Yup, but now there are flake8 errors, just add a space after each ,

ElDeveloper commented 2 years ago

@RodolfoSalido also, can you add a test for these changes?

RodolfoSalido commented 2 years ago

I can add that test.

-Rodolfo

Sent from my iPhone

On Jan 13, 2022, at 11:29 AM, Antonio Gonzalez @.***> wrote:

 @antgonza commented on this pull request.

I thought that this PR will have conflicts after merging #62 but it seem fine; which is surprising but kind of cool. Anyway, one comment; thank you.

In metapool/tests/test_metapool.py:

@@ -190,6 +190,18 @@ def test_read_pico_csv_spectramax(self): with self.assertRaises(ValueError): read_pico_csv(fp_spectramax, plate_reader='foo')

  • def test_read_pico_csv_spectramax_negfix(self):

  • Tests that Concentration values are clipped

  • to a range of (0,60), eliminating possible

  • negative concentration values.

  • fp_spectramax = os.path.join(os.path.dirname(file), 'data',
  • 'pico_spectramax.txt') Thank you @RodolfoSalido; what do you think about adding a small test to make sure that there are actually values out of the range in fp_spectramax? I tried to find them but it was a bit complicated for me.

— Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android. You are receiving this because you were mentioned.

RodolfoSalido commented 2 years ago

can we merge this ? Need someone to approve my new pico_spectramax.txt file.

antgonza commented 2 years ago

Just to be clear, you need to solve the conflicts via CLI in your local version; please email me if you need help with this.

RodolfoSalido commented 2 years ago

Ok, after many small edits, I think I got my local branch to have no conflicts with the base branch, and pass all tests. But I'm getting a check error that I'm not familiar with. Any help?

antgonza commented 2 years ago

Thank you @RodolfoSalido; I think the failure was a github actions hiccup that has been solved - I restarted the tests and now are passing.