KoslickiLab / YACHT

A mathematically characterized hypothesis test for organism presence/absence in a metagenome
MIT License
28 stars 7 forks source link

Error working with GTDB database bug #105

Closed mfl15 closed 6 months ago

sonarcloud[bot] commented 6 months ago

Quality Gate Passed Quality Gate passed

Issues
0 New issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

codecov[bot] commented 6 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (93d50f5) 71.53% compared to head (5b0687f) 71.57%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #105 +/- ## ========================================== + Coverage 71.53% 71.57% +0.04% ========================================== Files 11 11 Lines 1075 1073 -2 ========================================== - Hits 769 768 -1 + Misses 306 305 -1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

dkoslicki commented 6 months ago

@mfl15 I see just that block of code that @jsrdrgz added being removed. Does this affect #12? And/or has testing been added for the empty/small sketch problem and a test for #103 ?

mfl15 commented 6 months ago

@dkoslicki As Judith told, "This condition was not made for the old issue but a just in case condition for mean_abundance.", also her unit test passes for https://github.com/KoslickiLab/YACHT/issues/12.

condition of https://github.com/KoslickiLab/YACHT/blob/93d50f558e6a0d8f6b678eebb03475b23fe49c03/yacht/utils.py#L45-L48 looks enough to fix https://github.com/KoslickiLab/YACHT/issues/12, it was not removed.

I am planning to add testing of https://github.com/KoslickiLab/YACHT/issues/103 in scope of https://github.com/KoslickiLab/YACHT/issues/87. I will make the test with the same params as discussed in https://github.com/KoslickiLab/YACHT/issues/103

PR with test: https://github.com/KoslickiLab/YACHT/pull/106/

dkoslicki commented 6 months ago

@dkoslicki As Judith told, "This condition was not made for the old issue but a just in case condition for mean_abundance.", also her unit test passes for #12.

condition of

https://github.com/KoslickiLab/YACHT/blob/93d50f558e6a0d8f6b678eebb03475b23fe49c03/yacht/utils.py#L45-L48

looks enough to fix #12, it was not removed. I am planning to add testing of #103 in scope of #87. I will make the test with the same params as discussed in #103

PR with test: #106

Awesome, thanks for the details @mfl15 ! Looks good then