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

Add downsampling to select_sketch in rust #1292

Closed
luizirber opened this issue Jan 30, 2021 · 5 comments
Closed

Add downsampling to select_sketch in rust #1292

luizirber opened this issue Jan 30, 2021 · 5 comments
Labels
enhancement rust

Comments

@luizirber
Copy link
Member

Currently sig.select_sketch in Rust only return sketches that match exactly a template. But... If there is a scaled=10 sketch and the template is scaled=100, the sketch can be downsampled.

There is some code to do that in luizirber/2021-01-27-olga-remove-protein@e9603f3, but this triggers copies all the time. Might need to think better (maybe an extra parameter, downsample?) on how to control this behavior.
@ctb ctb changed the title Add downsampling to select_sketch Add downsampling to select_sketch in rust Aug 3, 2022
@ctb
Copy link
Contributor

ctb commented Sep 23, 2023

@luizirber I think this is being resolved in your new rust work, yes?

@luizirber
Copy link
Member Author

This wasn't quite covered in #2230. Exposing the Select trait was done, but the selecion is not doing the downsampling. This is the place that needs:

  1. proper error handling (not a generic MismatchKSizes!)
  2. do the downsampling, based on the selection

cc @bluegenes

@bluegenes bluegenes mentioned this issue Jan 17, 2024
Merged
@bluegenes
Copy link
Contributor

@luizirber - how would you suggest better handling the error?

Separately, do we also need to modify select_sketch for query downsampling (and allow downsampling within mh.check_compatible?

@luizirber
Copy link
Member Author

@luizirber - how would you suggest better handling the error?

Returning an empty sig as in #2931 sounds good!

Separately, do we also need to modify select_sketch for query downsampling (and allow downsampling within mh.check_compatible?

select_sketch should be deprecated and removed 🙃
and base everything on top of the Select trait instead.
So, I would say no need to fix it.

bluegenes added a commit that referenced this issue Jan 23, 2024
@bluegenes @luizirber
MRG: in core, enable downsample within select (#2931)
94b88cc 
Attempting #1292 in order to move forward
sourmash-bio/sourmash_plugin_branchwater#134

Modifies `Signature` `Select` to downsample automatically.

- for scaled sketches, while checking ksize, we also retain only
sketches that have the right scaled or can be downsampled (scaled <=
selection.scaled())
- next, we iterate through the sketches and downsample any where scaled
< selection.scaled()

Note that for `sourmash_plugin_branchwater` compatibility, we need:

- `byteorder` = "1.4.3"
- `wasm-bindgen` = "0.2.89"
- `once_cell` = "1.18.0"
---------

Co-authored-by: Luiz Irber <[email protected]>
@bluegenes bluegenes mentioned this issue Jan 23, 2024
Open
@bluegenes
Copy link
Contributor

Closing after merging #2931!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement rust
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@luizirber @ctb @bluegenes