Skip to content

feat(variant): retain unevaluated variants in a sideline container (#106) - #211

Closed
TimD1-bot wants to merge 1 commit into
105_td_subset-num-a-r-gfrom
106_td_add-sideline-container
Closed

feat(variant): retain unevaluated variants in a sideline container (#106)#211
TimD1-bot wants to merge 1 commit into
105_td_subset-num-a-r-gfrom
106_td_add-sideline-container

Conversation

@TimD1-bot

Copy link
Copy Markdown
Collaborator

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 #106. Step 6 of 8 toward #48. Stacked on #105 (105_td_subset-num-a-r-g); review that first.

parse_variants() discarded every record that failed FILTER or fell below --min-qual, so
nothing survived but an aggregate counter. Both are now retained and written to the summary VCF
as calls that were not evaluated.

Sideline container

ctgSideline is a per-contig, per-callset container of retained variants, keyed by
(record ordinal, haplotype). Both reasons wired up here precede the for (hap) loop, so
neither can partially drop a record; each stores SIDELINE_ALL_HAPS, leaving the per-allele
key for the reasons that arrive next.

Retained variants never enter ctgVariants, so every loop over poss/n in src/cluster.cpp,
src/dist.cpp, src/phase.cpp, and src/print.cpp is unreachable for them and "excluded from
all analysis" holds by construction, rather than by an ignored check at each of those sites
where one missed check would silently corrupt supercluster indices or precision/recall counts.

An entry carries the source POS/REF/ALT and GT verbatim, since nothing normalized or
split a retained record, plus the reason code that selects its FILTER tag.

FILTER tags

sideline_strs/sideline_descs in src/globals.cpp hold the tag and description of each
SIDELINE_* reason. region_strs could not be reused: its display values are space-padded
("OFF CTG"), which is not a legal FILTER ID.

Tag Trigger
VCFDIST_FAILED_FILTER FILTER holds none of the filters --filter selected
VCFDIST_LOW_QUAL QUAL is below --min-qual

Per the VCF spec FILTER lists the filters a record failed, so the tag joins the record's own
list; a lone PASS (or .) is replaced instead, since a record excluded from evaluation passed
nothing it was held to. Tags are VCFDIST_-prefixed against collision with input FILTER IDs and
uppercase to match the convention for FILTER IDs.

Writer

write_summary_vcf()'s merge walk grows from two streams to four, interleaving each callset's
retained records into the position-ordered walk. A retained record was excluded before any
comparison ran, so it is never matched: one sample holds its call, reporting BD=N — the GA4GH
decision for a call that was not assessed — and nothing else, and the other sample is entirely
missing. The header gains a ##FILTER line per reason and BD's description covers N.

INFO/FORMAT fields whose values index the source ALT list are dropped from a retained
record: it keeps the whole ALT list it was written with, so no single element is the one its
allele carries, while the propagated header declares those fields at the cardinality a split
record carries.

Other changes

Source column retention now runs before any filtering decision, so a record excluded from
evaluation still carries its own ID, QUAL, FILTER, INFO, and FORMAT. The
src_info/src_fmt_vals subsetting helpers became free functions so both containers can share
them. The two parse-time INFO lines now say "not evaluated" rather than "skipped".

Tests

  • Unit: one parse-time test per reason, plus record-scope inertness of the haplotype key, source
    column preservation, the lone-PASS replacement, and that neither an evaluated variant nor a
    record dropped for any other reason is retained. Writer tests pin BD=N, the empty companion
    sample, the FILTER contents with and without an input filter, and position interleaving.
  • Integration sideline: one run exercising both reasons, both with and without a non-PASS
    input FILTER on the same record.
  • Integration inertness: the same run against an input the three retained records were removed
    from by hand, diffing every analysis output. All eight are byte-identical.

Locally: 119 pytest tests pass (633 unit assertions), a clean -Wall -Wextra build adds no
warnings, and doxygen is clean.

@TimD1
TimD1 force-pushed the 105_td_subset-num-a-r-g branch from 2fadee3 to b9367b5 Compare August 8, 2026 02:01
@TimD1
TimD1 force-pushed the 106_td_add-sideline-container branch from a01dbdc to df9f7dd Compare August 8, 2026 02:01
@TimD1-bot

Copy link
Copy Markdown
Collaborator Author

Claude Opus 5 🤖: Planned rework, filed as #229.

A sidelined record is emitted verbatim, so once the writer is htslib-based this becomes bcf_dup(rec) + bcf_add_filter() + bcf_write(). That removes all of ctgSideline (~186 lines), which currently re-implements the same six src_* accessors as ctgVariants plus its own print_var, and replaces the manual filter + ";" + tag concatenation.

No action needed until #229 lands.

)

parse_variants() discarded every record that failed FILTER or fell below --min-qual, so
nothing survived but an aggregate counter. Both are now retained in a per-contig,
per-callset sideline container and written to the summary VCF as calls that were not
evaluated.

Retained variants never enter ctgVariants, so every loop over poss/n in clustering,
alignment, phasing, and reporting is unreachable for them and "excluded from all
analysis" holds by construction rather than by a check at each of those sites.

The writer's merge walk grows from two streams to four, interleaving each callset's
retained records by position. Records are built through the same htslib helpers the
evaluated ones use: set_record_qual/id/filters/info for the site columns, and
set_record_samples over a sample_fields pair for the two samples. A retained record
reports BD=N on the one sample that called it, and bcf_add_filter() adds the
VCFDIST_-prefixed tag naming why it was not evaluated, which displaces a lone PASS.

Nothing normalized or split a retained record, so it keeps the whole source ALT list and
every Number=A/R/G value indexing it, rather than being subset to one allele the way an
evaluated variant is. Its GT is likewise the source's own and may name any ALT ordinal.

Source columns are now retained before any filtering decision, so a retained record
still carries its own ID, QUAL, FILTER, INFO, and FORMAT.
@TimD1-bot
TimD1-bot force-pushed the 106_td_add-sideline-container branch from 8669bad to 6064810 Compare August 8, 2026 05:52
@TimD1-bot

Copy link
Copy Markdown
Collaborator Author

Claude Opus 5 🤖: #229 has landed (it is in this PR's base as d4d7149), so this is now actionable. Revisiting it against the merged writer:

Done. The hand-rolled parts are gone. ctgSideline::print_var and its manual field padding (dot_fields, missing_fixed_fields, the NO_CALL/NOT_EVALUATED dot strings) are replaced by set_var_record + var_sample_fields, which go through the same set_record_qual/set_record_id/set_record_filters/set_record_info and set_record_samples helpers the evaluated records use. The filter + ";" + tag concatenation is gone too: bcf_add_filter() documents "if other than PASS, existing PASS is removed", which is exactly the rule that was being hand-written. Five of the six src_* accessors are also gone — nothing here subsets or reformats a retained column, so the store is read where it is written.

Not done: replacing the container with a bcf_dup() of the input record. Two things block it, both discovered by trying it against the merged writer rather than against the pre-#229 one:

  1. The sample half cannot be copied. An input record has one sample; the summary VCF has two, and every record must carry all 15 fixed FORMAT fields against the output header's declarations. So var_sample_fields() has to be built either way — a dup only supplies the site columns.
  2. A dup would corrupt FORMAT on collision. keep_format() deliberately drops input FORMAT keys that collide with the writer's fixed list, and retain_header_lines() does not declare them. An input carrying its own GQ, PS, or SC would, after bcf_translate(), land in the output's identically-named fixed slot. The text store is what keeps those separated.

There is also a memory point: a bcf1_t per retained record is far heavier than the six strings, and a --filter-heavy run can retain millions.

So the ~186-line removal isn't reachable, but the duplication it was aimed at is: ctgSideline is down to add, set_var_record, var_sample_fields, and its storage. Net -79 lines against the pre-rebase version of this branch, on top of what the htslib port already removed.

One behavior change worth calling out, which #229 made correct rather than my doing: a retained record now keeps its whole source ALT list and its Number=A/R/G values unsubset. Since #105 stopped rewriting the propagated declarations, a verbatim multi-allelic record is now self-consistent — AF=0.4,0.6 beside ALT=C,G at Number=A, with GT=1|2 still naming a real allele. The previous revision dropped those fields. Pinned by WriteSummaryVcf.MultiAllelicRetainedRecordKeepsEveryAllele.

@TimD1 TimD1 closed this Aug 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants