[OC-832] Fix dropped sample/mapping rows on merge; tighten mergesqlite checks - #420
Open
cvaske wants to merge 2 commits into
Open
[OC-832] Fix dropped sample/mapping rows on merge; tighten mergesqlite checks#420cvaske wants to merge 2 commits into
cvaske wants to merge 2 commits into
Conversation
…e checks - fix mergesqlite() dropping sample and mapping rows for any variant that already exists in the merged output (uid_dic was never populated for the skip-duplicate-variant case, so later sample/mapping inserts for that uid were silently skipped) - extend the pre-merge consistency check beyond variant/gene_header column names to also cover sample_header/mapping_header columns and (name, version) pairs for variant_annotator/gene_annotator across all input dbs; error out naming the mismatch before doing any merge work - recompute "Number of unique input variants" and "Result modified at" in the merged db's info table instead of leaving db 1's stale values - detect colliding sample_id values across input dbs and error out by default - add an opt-in `path:label` argument suffix to rename a file's sample_ids to `label__sample_id` to resolve a collision - add tests/test_mergesqlite.py covering all of the above
Four correctness bugs in mergesqlite() could silently drop, duplicate, or misattribute merged sample/mapping rows: - new_uid running counter was clobbered by the Sample/Mapping blocks reusing it as a scratch variable, causing duplicate base__uid values and misattributed sample rows when merging 3+ dbs. - variant_id() concatenated chrom+pos+ref+alt with no delimiter, so distinct variants (e.g. chr=1/pos=234 vs chr=12/pos=34) could collide and get silently merged onto the wrong variant. Now delimited with ':'. - fileno_dic only recorded entries for newly-seen input filepaths, raising KeyError when a later db repeated an earlier db's input path; now maps repeated filepaths onto their existing fileno. - labels was a dict keyed by dbpath, so passing the same physical file twice with different :label suffixes silently dropped the first label; now a list parallel to dbpaths. - mergesqlite_check_info()'s column consistency check now also checks order of columns, which matehes the merging logic used later on. Added regression tests for all five, each confirmed to fail against the prior behavior before the fix.
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
1 participant
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.
Overview
Improve mergesqlite such that we can use it as part of parallelization of OpenCravat (e.g. in nextflow pipelines). This fixes bugs in the current variant/sample mapping, performs additional sample name checks, and provides ways to map sample names to the FILENAME__SAMPLE format that multi-file
oc runcommands use.Changes
path:labelargument suffix to rename a file's sample_ids tolabel__sample_idto resolve a collisionTests
pytest tests/test_mergesqlite.py -v