feat(variant): add per-variant provenance and ploidy vectors by TimD1-bot · Pull Request #116 · TimD1/vcfdist

TimD1-bot · 2026-08-03T15:52:15Z

Note

Authorship: the content below was drafted by Claude Opus 5 (an AI coding agent) and
filed via gh under @TimD1-bot, a bot account operated by @TimD1. It reflects the
agent's analysis, not a statement authored by @TimD1.

Closes #101.

Adds the three per-variant vectors that the rest of #48 reads. Inert: nothing consumes them
yet, and summary.vcf is unchanged.

Changes

src/variant.h — rec_idxs, alt_idxs, and ploidies on ctgVariants, in the
"originally parsed data (size n)" block.
src/variant.cpp — set in the explicit add_var, copied in add_var(other_vars, idx),
passed at all three parse_variants call sites including both CPX halves.
src/cluster.cpp — threaded through all four merged_vars->add_var call sites in
load_and_merge_callset_vars_across_haps.
tests/unit/src/test_variant.cpp — new, 8 cases over a synthetic in-memory VCF fixture.

Notes for review

The three parameters are inserted after phase_set, before supercluster, rather than
appended. Appending would force the parse_variants call sites to spell out 12 intervening
defaults. Mid-signature insertion into a defaulted parameter list is only safe because no
existing caller passes supercluster or later positionally — the three add_var calls in
variantData::add_variants (src/variant.cpp:626, :641, :655) stop at phase_set and so
take the new defaults. Worth a second pair of eyes.
rec_idx = n - 1, where n is the file record counter incremented at
src/variant.cpp:865. That increment precedes every filter continue, so the ordinal counts
records as read from the file rather than records retained — which is what the retained-record
store in D3: Retain source records and propagate ID/QUAL/FILTER/INFO/FORMAT #104 needs.
ploidy = std::abs(ngt) is safe here because ngt <= 0 is already rejected upstream
(src/variant.cpp:918, :924) and the parser hard-errors on multi-sample VCFs, so ngt is
the ploidy. This matches how the existing per-contig check at src/variant.cpp:933 derives it.
Sentinels are -1 / -1 / 0 for unknown. variantData::add_variants has no callers in
src/ today and takes them.

Verification

cd src && make — clean, apart from the pre-existing dist.cpp:1070 unused-parameter warning.
./test_vcfdist — 9 tests from 3 suites, all pass.
pytest -vv from tests/ — 3 passed, integration test included (the chr20 GRCh38 reference
was present).
Inertness: vcfdist run on the chr20 fixture before and after; summary.vcf, query.tsv,
truth.tsv, and phase-blocks.tsv are byte-identical across all ~18 MB. Only ##CL= differs,
and only when the two runs are invoked from different working directories, since that header
records the command line.
Mutation-tested the new assertions by hardcoding rec_idx=0, ploidy=2, alt_idx=1: 4 of the
5 provenance tests fail, confirming they are not vacuous.

Add rec_idxs, alt_idxs, and ploidies to ctgVariants, sized n alongside the other parsed-data vectors, recording each variant's source VCF record ordinal, original 1-based ALT ordinal, and own ploidy from std::abs(ngt). Threaded through both add_var overloads and all four merged_vars->add_var call sites in load_and_merge_callset_vars_across_haps. Both halves of a decomposed CPX allele share one alt_idx, since they derive from the same original allele. Nothing reads the new vectors yet; summary.vcf is byte-identical. Unit tests in tests/unit/src/test_variant.cpp pin the values for a hom SNP, a het SNP, a 1|2 multiallelic record, a haploid record, and the shared alt_idx of a CPX record's INS and DEL halves.

…ness The harness added in #118 supplies GlobalsGuard, TempDir, and write_tmp_vcf, and already lists test_variant.o in the unit-test Makefile. The provenance tests now live in tests/unit/src/test_variant.cpp and use those helpers, so this branch no longer touches the Makefile and no longer hand-rolls mkstemp scaffolding.

The harness added in #118 supplies GlobalsGuard, TempDir, write_tmp_vcf, and make_fasta, and already lists test_variant.o in the unit-test Makefile. The filtering tests move into tests/unit/src/test_variant.cpp and use those helpers, dropping the hand-rolled temp-file and FASTA scaffolding. This reverts the earlier rename to test_variant_filtering.cpp: a separate file would require a Makefile edit, which the harness exists to avoid. Both this and PR #116 now append to test_variant.cpp, so whichever merges second resolves a small append conflict.

Closes #128. add_var() defaults every parameter after phase_set, so make_ctgVariants() passing var.supercluster in the eleventh position bound it to rec_idx once #116 inserted the three provenance parameters mid-signature, and supercluster silently took its -1 default. Both are int, so it compiled clean under -Wall -Wextra. var_desc gains rec_idx, alt_idx and ploidy fields (defaults -1, -1, 0) and all four are now passed explicitly, so tests can exercise the new per-variant vectors. The new fields are appended rather than inserted, so positional brace-initialization of var_desc keeps binding to the same members. MakeCtgVariants.ProvenanceFieldsReachTheirOwnVectors gives each of the five trailing fields a distinct value, so a future positional shift fails a test instead of passing silently. Also corrects var_desc::qual's doc comment, which claimed add_var() clamps it to g.max_qual: that holds for var_qual only, not gt_qual (#135). No behavior change in src/variant.cpp.

…115) Adds 78 cases to tests/unit/src/test_variant.cpp for the pure logic in variant.cpp: the ctgVariants constructor, both add_var overloads, get_vartype, set_allele_errtype, calcgt_is_swapped, var_on_hap, set_var_calcgt_on_hap, and the variantData constructor. Boundary and error paths are pinned explicitly: get_vartype sets g.sv_threshold rather than relying on the default, add_var pins the var_qual upper clamp with no lower clamp, set_allele_errtype asserts ac_errtype[vi] is stored on every path, and set_var_calcgt_on_hap enumerates all 16 diploid transitions plus both default-branch errors. Rebased onto the rec_idx/alt_idx/ploidy parameters #116 inserted ahead of supercluster in add_var: the three explicit-argument call sites pass and assert them, and CtgVariantsCtor.ZeroesN, AddVar.LaneLengthsTrackN and AddVarCopy.Roundtrip cover the three new per-variant vectors. Roundtrip still needs extending for strata_bits (#47) and is_phased (#46). The provenance cases from #116 are kept unchanged, moved below the add_var groups so the file stays grouped by function under test.

…115) (#124) Adds 78 cases to tests/unit/src/test_variant.cpp for the pure logic in variant.cpp: the ctgVariants constructor, both add_var overloads, get_vartype, set_allele_errtype, calcgt_is_swapped, var_on_hap, set_var_calcgt_on_hap, and the variantData constructor. Boundary and error paths are pinned explicitly: get_vartype sets g.sv_threshold rather than relying on the default, add_var pins the var_qual upper clamp with no lower clamp, set_allele_errtype asserts ac_errtype[vi] is stored on every path, and set_var_calcgt_on_hap enumerates all 16 diploid transitions plus both default-branch errors. Rebased onto the rec_idx/alt_idx/ploidy parameters #116 inserted ahead of supercluster in add_var: the three explicit-argument call sites pass and assert them, and CtgVariantsCtor.ZeroesN, AddVar.LaneLengthsTrackN and AddVarCopy.Roundtrip cover the three new per-variant vectors. Roundtrip still needs extending for strata_bits (#47) and is_phased (#46). The provenance cases from #116 are kept unchanged, moved below the add_var groups so the file stays grouped by function under test. Co-authored-by: Tim Dunn <timdunn@fulcrumgenomics.com>

add_var took 27 positional parameters, 17 of them defaulted, so inserting a parameter anywhere but the end silently rebound every argument after it. #116 had already shipped that bug once: a caller's supercluster argument became rec_idx and compiled clean under -Wall -Wextra. Every field now travels in a var_fields aggregate, matched by designator instead of position, with the 12 per-haplotype fields nested as hap[HAPS] so a hap1/hap2 transposition is no longer expressible. Members with no default are required: omitting one is a build failure under the newly added -Werror=missing-field-initializers (GCC; clang does not diagnose it). The four cluster.cpp merge sites hand-copied 13 parallel vectors each, which is the shape that produced the #116 miswiring; they now read one var_fields through the new ctgVariants::get_var accessor. Pure refactor: the chr20 fixture's 12 output files are byte-identical.

add_var took 27 positional parameters, 17 of them defaulted, so inserting a parameter anywhere but the end silently rebound every argument after it. #116 had already shipped that bug once: a caller's supercluster argument became rec_idx and compiled clean under -Wall -Wextra. Every field now travels in a var_fields aggregate, matched by designator instead of position, with the 12 per-haplotype fields nested as hap[HAPS] so a hap1/hap2 transposition is no longer expressible. Members with no default are required: omitting one is a build failure under the newly added -Werror=missing-field-initializers (GCC; clang does not diagnose it). Call sites name the type -- add_var(var_fields{...}) rather than add_var({...}) -- because GCC 13.3, the compiler on ubuntu-24.04 and so on CI, rejects a bare designated-initializer list as a function argument once any member is initialized from a non-constant expression. Naming the type sidesteps that and still enforces the required members. -std=c++20 does not help, so the C++17 standard level is unchanged. The four cluster.cpp merge sites hand-copied 13 parallel vectors each, which is the shape that produced the #116 miswiring; they now read one var_fields through the new ctgVariants::get_var accessor. Pure refactor: on the chr20 fixture every output is unchanged, the two files recording the invocation aside.

add_var took 27 positional parameters, 17 of them defaulted, so inserting a parameter anywhere but the end silently rebound every argument after it. #116 had already shipped that bug once: a caller's supercluster argument became rec_idx and compiled clean under -Wall -Wextra. Every field now travels in a var_fields aggregate, matched by designator instead of position, with the 12 per-haplotype fields nested as hap[HAPS] so a hap1/hap2 transposition is no longer expressible. Members with no default are required: omitting one is a build failure under the newly added -Werror=missing-field-initializers (GCC; clang does not diagnose it). Call sites name the type -- add_var(var_fields{...}) rather than add_var({...}) -- because GCC 13.3, the compiler on ubuntu-24.04 and so on CI, rejects a bare designated-initializer list as a function argument once any member is initialized from a non-constant expression. Naming the type sidesteps that and still enforces the required members. -std=c++20 does not help, so the C++17 standard level is unchanged. The four cluster.cpp merge sites hand-copied 13 parallel vectors each, which is the shape that produced the #116 miswiring; they now read one var_fields through the new ctgVariants::get_var accessor. Pure refactor: on the chr20 fixture every output is unchanged, the two files recording the invocation aside. Co-authored-by: Tim Dunn <timdunn@fulcrumgenomics.com>

TimD1-bot mentioned this pull request Aug 3, 2026

fix(variant): accurate counters and WARNs for parse-time dropped variants #117

Merged

TimD1-bot mentioned this pull request Aug 3, 2026

D3: Retain source records and propagate ID/QUAL/FILTER/INFO/FORMAT #104

Closed

TimD1 merged commit 18bf400 into dev Aug 3, 2026
1 check passed

TimD1 deleted the 101_td_D3-per-variant-provenance-vectors branch August 3, 2026 16:50

This was referenced Aug 3, 2026

make_ctgVariants: supercluster silently passed as rec_idx since #116 inserted params mid-signature #128

Closed

Harden add_var's 26-parameter signature against silent positional rebinding #136

Closed

TimD1-bot mentioned this pull request Aug 3, 2026

refactor(test): simplify GlobalsGuard and TempDir, and test the harness #122

Merged

TimD1-bot mentioned this pull request Aug 3, 2026

test(unit): cover variant genotype, allele-count, and vartype logic (#115) #124

Merged

This was referenced Aug 4, 2026

test(unit): add cluster index and range arithmetic tests #125

Merged

refactor(variant): pass add_var a named var_fields struct (#136) #190

Merged

This was referenced Aug 5, 2026

refactor(variant): retire per-contig ploidy in favor of per-variant ploidy (#102) #204

Merged

D3: Retire per-contig ploidy in favor of per-variant ploidy #102

Closed

Navigation Menu

feat(variant): add per-variant provenance and ploidy vectors - #116

feat(variant): add per-variant provenance and ploidy vectors#116
TimD1 merged 2 commits into
devfrom
101_td_D3-per-variant-provenance-vectors

TimD1-bot commented Aug 3, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

TimD1-bot commented Aug 3, 2026

Changes

Notes for review

Verification

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants