-
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
downsample_*
functions in minhash.rs _always_ downsample, even when downsampling is not necessary
#3343
Comments
From my question on slack: Hi all, a Rust conundrum that I don’t quite know how to resolve. Maybe writing it out will help. In my ideal world, we would change this function to only return a new sketch if it has to change the sketch, but would return itself (unmodified) if not - if the requested max_hash is the same as the current one, there’s no need to create a new one. Now obviously you can’t have a function return both KmerMinHash and &KmerMinHash. So how do I do this, structurally? Is this a place where Clone on Write would be useful? You can see what I had to do in a function that uses these functions, here: the call to query.inflated_abundances is the only thing that is different in the if/else statement between doing the downsampling and not - that’s because against_ds here is a new copy in the if branch, while we’re using against in the else. |
luiz writes: CoW would be a solution, but what about changing the fn sig to
? This avoid possible lifetime issues by giving control to whoever calls the function then the check for downsampling or not would be done inside the downsample_max_hash function and if nothing needs to be done, just return self kind of how select() works sourmash/src/core/src/manifest.rs Line 262 in 34001e7
|
resolved in #3342 |
The root cause of the 10x slowdown in the branchwater plugin (sourmash-bio/sourmash_plugin_branchwater#463) was that
downsample_scaled
was being called on everyagainst
sketch, and it was creating a new sketch every time it was called - even when no downsampling needed to be done, because the scaled values match.This is fixed in sourmash-bio/sourmash_plugin_branchwater#464 by only doing the downsampling if the scale does not match.
I think the sourmash code base behavior is somewhat unexpected and we should avoid doing expensive work when it's not needed. But I'm not sure how to modify the function signatures appropriately.
The text was updated successfully, but these errors were encountered: