DNA moltype output in manifest is dna not DNA
#49
Comments
ctb
added a commit
to sourmash-bio/sourmash
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.
|
This was fixed in sourmash-bio/sourmash#3193. We now need to wait for a new release of sourmash-rs core, r0.14.0, and then update |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
When I run the quickstart and then unzip the zip file and look at the manifest,
I see that the moltype is
dna, notDNA.It should be
DNA, all caps.This appears to be one of the things confusing sourmash over in sourmash-bio/sourmash#3191.
I'll discuss the sourmash fails here over in that issue, but the moltype fix maybe belongs here? I can't quite figure out where it's happening - maybe the Rust manifest code in sourmash needs to be fixed?
The text was updated successfully, but these errors were encountered: