-
Notifications
You must be signed in to change notification settings - Fork 80
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
[MRG] require scaled signatures for containment #1381
Conversation
Codecov Report
@@ Coverage Diff @@
## latest #1381 +/- ##
==========================================
+ Coverage 88.84% 94.16% +5.32%
==========================================
Files 123 96 -27
Lines 18270 14707 -3563
Branches 1409 1410 +1
==========================================
- Hits 16232 13849 -2383
+ Misses 1800 621 -1179
+ Partials 238 237 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Ready for review @luizirber @bluegenes |
yay, tests pass! |
🎉 |
I stumbled across some code that shouldn't have worked while writing a new test for #1374, and discovered that we are allowing
contained_by
to be called onnum
MinHash objects.This is unambiguously wrong and I think we should remove it from the code base. Only
scaled
MinHashes allow accurate estimation of containment. I guess we didn't know that a few years back, though, so it crept into the code base :).#1345 is relevant - but while there are reasonable places to call
count_common
on regular MinHashes, there are no reasonable places to callcontained_by
, AFAICT.NOTE: this breaks backwards compatibility, but I think it's rectifies a bug so we're ok.
Checklist
make test
Did it pass the tests?make coverage
Is the new code covered?without a major version increment. Changing file formats also requires a
major version number increment.
changes were made?