RocksDB-based RevIndex gather functionality is broken.
#3139
Comments
2 tasks
bluegenes
pushed a commit
that referenced
this issue
May 10, 2024
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 - [x] do we need to fix same problem in `linear.rs`? or just rename things per #3137? - [x] we should add some tests for this
1 task
ctb
added a commit
that referenced
this issue
Jun 9, 2024
…d by plugins (#3193) This PR fixes a bug in `disk_revindex.rs::RevIndexOps::gather` where RocksDB-based `gather` was incorrectly subtracting hashes multiple times from the counter in situations of high redundancy. For example, consider this Venn diagram of the 3-way intersection between three sketches:  When a metagenome contains the union of all three of these sketches, the broken implementation would subtract the `10` at the center multiple times. This was caused by removing hashes from the matches, rather than the intersection, each pass through the counter. Of note, this made RocksDB-based `fastmultigather` return incorrect results, ref sourmash-bio/sourmash_plugin_branchwater#322; first discovered in #3138 (comment). The PR fixes this, and adds a more complete pair of tests, based on `test_gather_metagenome_num_results` in the Python tests for sourmash. This PR also adjusts the hash function string for DNA sketches in Rust to be uppercase `DNA` rather than lowercase `dna`, ref sourmash-bio/sourmash_plugin_directsketch#49 And remember, it's not *just* the destination - it's the friends you make along the way, like `env_logger`. * Fixes #3139 * Fixes sourmash-bio/sourmash_plugin_directsketch#49 For consideration: Right now we are calculating the intersection twice, once in `disk_revindex.rs` and once in `calculate_gather_stats` in `revindex/mod.rs`. This is unnecessary, of course. But the function signature for `calculate_gather_stats` would need to change to take the intersection as an argument. We could: * keep calculating it twice, just for simplicity; * change `calculate_gather_stats` to take an _optional_ intersection, and calculate it if not provided; * change `calculate_gather_stats` to require an intersection. TODO: - [x] add at least one explicit test for the moltype fix Other notes: * there is a discrepancy between the Python (`sourmash gather`) and Rust (`sourmash_plugin_branchwater` results) calculations for `remaining_bp`. It seems to me like the Python one is definitely wrong; not yet sure about Rust. Viz #3194.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
See sourmash-bio/sourmash_plugin_branchwater#322 for details.
The relevant Rust test is:
revindex_load_and_gather_2()The text was updated successfully, but these errors were encountered: