-
Notifications
You must be signed in to change notification settings - Fork 37
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Correct FP rounding errors #539
base: master
Are you sure you want to change the base?
Correct FP rounding errors #539
Conversation
Codecov Report
@@ Coverage Diff @@
## master #539 +/- ##
==========================================
+ Coverage 82.99% 83.01% +0.01%
==========================================
Files 132 132
Lines 29621 29621
==========================================
+ Hits 24585 24590 +5
+ Misses 5036 5031 -5
Continue to review full report at Codecov.
|
I can't explain the codecov/patch problem. Is it a deal breaker for merging? |
0% coverage for patch, should mean that you changes are not covered by tests at all. It would be great if you could add tests covering those cases. |
I always do, but in this case I just changed 3 logical operators in existing code, so I don't see how coverage could change at all. I read some issues in codecov forums echoing similar strange behavior with 0% codecov/patch but I can't tell if it's related. |
The coverage has not changed (what can be seen on codecov/project check). The problem is that the code you've changed is not covered by tests at all. PS: I'm not saying this should block accepting this, I'm just explaining why Codecov complains. |
I see, so it was uncovered all along? |
Exactly. You've touched code paths which are not tested at all. It's not that hard as there is about 30% of that file not covered by tests, see https://codecov.io/gh/whoosh-community/whoosh/src/master/src/whoosh/matching/binary.py |
Ah I see! Good opportunity to add unit tests then. I'll try to get to that. Thanks for the clarification. |
I'm unable to find a unit test that covers this code path at this time. Given that this patch fixes a found bug with minimal changes, I propose it be merged anyway. We can work on increasing code coverage separately. |
Following up on #519, correcting a few more rounding bugs I found.