Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix rust Signature API for minhashes method. #1167

Closed
ctb opened this issue Aug 14, 2020 · 2 comments · Fixed by #2329
Closed

fix rust Signature API for minhashes method. #1167

ctb opened this issue Aug 14, 2020 · 2 comments · Fixed by #2329
Assignees
@mr-eyes
Labels
rust

Comments

@ctb
Copy link
Contributor

ctb commented Aug 14, 2020

see #1159 (comment) and follow-on.
@ctb ctb added the rust label Aug 14, 2020
@ctb ctb mentioned this issue Aug 14, 2020
Merged
5 tasks
@ctb
Copy link
Contributor Author

ctb commented Aug 16, 2020

worked on exposing this error more cleanly --

in test_sourmash_sketch.py::test_dna_defaults(), I added the following code at the end:

siglist = factory()
sig = siglist[0]

then:

sig.minhash

yields

E       sourmash.exceptions.Panic: sourmash panicked: thread 'unnamed' panicked with 'not implemented' at src/core/src/ffi/signature.rs:172

and

minhashes = sig.minhashes()

causes a segmentation fault.

@luizirber
Copy link
Member

For whoever ends up implementing this: rename the .minhashes method to .sketches instead =]

@ctb ctb mentioned this issue May 3, 2021
Merged
6 tasks
@ctb ctb mentioned this issue Feb 3, 2022
Merged
@mr-eyes mr-eyes self-assigned this Feb 3, 2022
@ctb ctb mentioned this issue Oct 14, 2022
Merged
@ctb ctb closed this as completed in #2329 Oct 15, 2022
ctb added a commit that referenced this issue Oct 15, 2022
@ctb
[MRG] fix Signature.minhash API during sourmash sketch (#2329)
2198b32 
Addresses #1167 by fixing `signature_first_mh` in the signature FFI to
support both `KmerMinHash` and `KmerMinHashBTree`; this way, sketches
generated by the Python `_signatures_for_sketch_factory` (which are
BTree sketches in Rust) can be passed back to
Python.

Specifically, this PR:
* implements `From<&KmerMinHashBTree>` for `KmerMinHash`;
* adds an arm to the match statement in the FFI function to catch
`KmerMinHashBTree` structs and convert them into `KmerMinHash` structs
suitable for returning to Python;
* provides a default match arm to catch (e.g.) `HyperLogLog` and raise
an error back to Python reporting that it found an unsupported sketch
type;
* adds a little to the Python tests to prevent backsliding.

Note that I don't _think_ this does any more copying of sketches than
the current codebase does.

And, on that halcyonic day when the FFI properly wraps
`KmerMinHashBTree`, this code can be replaced by returning the proper
Rust object without conversion.

Fixes #1167.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@mr-eyes mr-eyes

Labels
rust
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[MRG] fix Signature.minhash API during sourmash sketch sourmash-bio/sourmash
3 participants
@luizirber @ctb @mr-eyes