-
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] Add FrozenSourmashSignature
#1610
Conversation
Codecov Report
@@ Coverage Diff @@
## latest #1610 +/- ##
==========================================
+ Coverage 84.70% 92.10% +7.40%
==========================================
Files 131 100 -31
Lines 15544 11343 -4201
Branches 2219 2236 +17
==========================================
- Hits 13166 10448 -2718
+ Misses 2085 601 -1484
- Partials 293 294 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
@luizirber opinions? |
…o add/frozen_signature
…o/sourmash into add/frozen_signature
…o add/frozen_signature
…o add/frozen_signature
…o add/frozen_signature
…o add/frozen_signature
FrozenSourmashSignature
FrozenSourmashSignature
Ready for review & merge @sourmash-bio/devs ! |
…o add/frozen_signature
hi @luizirber I think this PR is best reviewed by you, if you have time. It's a quick read, I promise ;). And you might have a suggestion for how to improve, too - I'm not thrilled with the context manager syntax I use. |
I have a bunch of commits in #2230 that flip the MinHash to default to large ones (b-tree based) and expose the Frozen ones (vec-based), should I add the commits here or make a new PR merging into this one? (I can also just merge them together in #2230, since that will be a breaking change version bump on the Rust side and I would like to avoid doing two in a row 😬 ) |
your call - if last option is easiest, go for it! |
I tested out with the pile of changes coming in #2230 and it works. In the future might want to avoid all the copying, but as you pointed in the comment for the context manager it is hard to track bugs otherwise... Another future optimization is being more strict on the Mutable/Frozen on the Rust side, right now they map to
|
awesome, thanks!! should I merge, then? |
Yup, go for it! |
🎉 |
I would have put more effort in to this, but note that |
Adds a
FrozenSourmashSignature
class, and provides sensibleto_mutable()
andto_frozen()
methods onSourmashSignature
andFrozenSourmashSignature
. Provides anupdate()
context manager that wraps changes so that aFrozenSourmashSignature
is left at the end.Complement to #1508, which did this with
MinHash
.As with #1508, very few changes are required to make all the tests pass. The ease of doing this suggests to me that this is a Good Change.
Also as with #1508, I really like this updated code; it's so nice and clear!
I am hoping that these changes make it easier for @luizirber in Rust/Python interaction.
On the other hand, this could break some 3rd party code (as #1508 did :). Admittedly we are the owners of most of that code, but 🤷 not sure if we should delay this 'til 5.0. On the other hand, could fit nicely with a 4.5.0 release.
TODO:
flatten
anddownsample
(b/c signatures are already flat and/or downsampled)SourmashSignature
by default owns a frozenMinHash
(I think it does, but confirm)SourmashSignature.to_frozen()
also freezes theMinHash
object.ss = ss.to_mutable(); ss.minhash = xyz; ss = ss.to_frozen()
dance;with sig.mutate() as sig:
?into_frozen()
andinto_mutable()
to_mutable()