fix: verify reconstructed sequences by genome name - #193
Open
mmolari wants to merge 5 commits into
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Unifies sequence verification across
build,mergeandreconstructbehind one name-keyed helper. Covers §4.4 ofdev/design/merge.md.Matching by name is the only option left once
mergerenumbers path ids: neither record order norFastaRecord::indexcan pair genomes up. The §4.3 checks from the previous PR make names a valid key, since they are now guaranteed unique.This turned out to fix two latent bugs rather than just unify style:
build --verifypanicked. It indexed&fastas[actual.index], valid only when the input records' indices are exactly0..n-1. Any programmatically assembled record set breaks that — including this repo's own test helper, whose right half carries indices[3, 4, 5]for a 3-record slice. Every existing test escaped it by passingverify=false, sobuild(.., verify=true)had no coverage at all.reconstruct --verifywas unreliable. Records were paired positionally, so a verification file with surplus records silently passed (exit 0); too few gave a misleadingexpected length 0 but got N; and a merged graph failed spuriously withexpected length N but got Nat identical lengths, because wholeFastaRecords were compared andindexdiffers after relabeling.pangraph/reconstruct.rsis a new home for reconstruction (it is a graph operation —pangraph/pangraph.rswas reaching up intocommands::) and holds the shared API:path_ids_by_name,reconstruct_genome,reconstruct_by_name,sequences_by_name,verify_genome,verify_graph_sequences.compare_sequencesis deleted.verify_graph_sequencestakes aGenomeCoverageargument, which was not in the original §4.4 sketch: build's intermediate clade graphs hold only their own clade's genomes and needPartial, while the final build graph, a merged graph andreconstruct --verifyrequireComplete. Making the final build checkCompleteis a small gain — nothing previously noticed a genome going missing.reconstruct --verifykeeps streaming rather than adopting the map: it is the only one of the three that does not otherwise need every genome resident, and a map would make peak memory proportional to total genome length (~2.5 GB for 500 E. coli genomes, vs ~10 MB today). It sharesverify_genomeand its error formatting.Errors now name the genome and, for equal-length differences, the first differing position with context:
13 unit tests and 7 new integration tests, including the headline case (verify a merged graph against its genomes in reversed order) and an explicit regression test for the
build --verifypanic.docs/docs/reference.mdregenerated for thecheck-cli-docsCI gate.Also fixes a pre-existing break caught by
cargo clippy --release: the merge integration tests calledsanity_check, which is#[cfg(any(test, debug_assertions))]and so does not exist for integration tests in release mode.