MRG: use correct denominator in f_unique_to_query

[ctb]

This PR fixes an issue introduced in #2943 where we introduced a subtly broken calculation that uses the current size of the query metagenome as the denominator for the f_unique_to_query calculation.

Fixes #3137

This PR also adds some commented-out test code that demonstrates #3139 / sourmash-bio/sourmash_plugin_branchwater#322. That's something I haven't been able to debug, so I'd suggest fixing that independently - I'd rather fix a problem now, rather than waiting until we can fix multiple problems at some later indeterminate time :).

Notes

• do we need to fix same problem in linear.rs? or just rename things per Erroneous f_unique_to_query calculation in Rust code. #3137?
• we should add some tests for this

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.65%. Comparing base (880b488) to head (14f9c3b).

Additional details and impacted files

@@            Coverage Diff             @@
##           latest    #3138      +/-   ##
==========================================
+ Coverage   86.62%   86.65%   +0.02%     
==========================================
  Files         136      136              
  Lines       15818    15818              
  Branches     2713     2713              
==========================================
+ Hits        13703    13707       +4     
+ Misses       1805     1801       -4     
  Partials      310      310              

Flag	Coverage Δ
hypothesis-py	25.36% <ø> (ø)
python	92.32% <ø> (ø)
rust	62.09% <100.00%> (+0.13%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[bluegenes]

this change looks good. There is a test for the calculate_gather_stats function in the same file, but it doesn't have a query subtraction (just the initial calculation), so it doesn't reveal the issue fixed here.

I didn't originally realize - the revindex tests are here:

sourmash/src/core/src/index/revindex/mod.rs

Line 446 in fe4790f

#[cfg(test)]

that would be a good spot to add tests

[ctb]

I did not know this: you have to specifically run the tests with cargo test --features branchwater or maybe cargo test-all-features in order to run the revindex tests!

(Our github actions does the right thing, I think. I just was struggling to get cargo test to fail ;)

[bluegenes]

I did not know this: you have to specifically run the tests with cargo test --feature branchwater or maybe cargo test-all-features in order to run the revindex tests!

This confused me at first too + I regularly need to remind myself of cargo testing syntax. Do we have any sourmash rust dev docs yet?

[ctb]

the code in linear.rs doesn't seem to get run at all by the tests, and in any case doesn't calculate most of the stats. So I think we can safely ignore it. Before merging this, I'll make sure there's an appropriate issue update elsewhere (either a new issue, or updating an existing issue, gotta go check to figure out which is appropriate.)

[ctb]

note that when I started adding tests, I got problematic results per #3139 / sourmash-bio/sourmash_plugin_branchwater#322.

The current tests do check the denominator issue in #3137 so I think we should merge it and leave #3139 to another PR. Let me know if you disagree @bluegenes !

[ctb]

Ready for review & (assuming tests pass) merge!

[bluegenes]

lgtm!