Skip to content

Name the cause when gen-seq-error-model has no weight to fit (#767) - #817

Merged
joshfactorial merged 3 commits into
developfrom
fix/767_zero_weight_distributions
Oct 5, 2026
Merged

joshfactorial merged 3 commits into
developfrom
fix/767_zero_weight_distributions

Conversation

@joshfactorial

@joshfactorial joshfactorial commented Oct 5, 2026 •

Copy link
Copy Markdown
Collaborator

The #765 audit traced two inputs to gen-seq-error-model that could still fail as a bare InvalidWeights error. Both are reachable, and each now fails with a message naming the cause.

Versioning: PATCH-level under versioning.md. No config key, output format or model format changes. Inputs that failed still fail, now with a message naming the cause.

1. MD tags that disagree with the reads

Reproduced: a BAM whose MD tag names a mismatch where SEQ has the reference base (MD 0A3 on read ACGT) failed with InvalidWeights([0.0, 0.0, 0.0, 0.0]).

Cause: TransitionObserver counted every MD mismatch position as a (reference → read) substitution without checking that the bases differ, so a stale tag recorded a self-transition. The matrix builder zeroes the diagonal, so a row holding only those had no weight left. One stale row was enough to fail an otherwise good fit.

Fix: such positions are tallied (md_seq_disagreements), and any disagreement is a hard error. The error gives the count of stale positions and of consistent mismatches, says the tags cannot be trusted, and points to samtools calmd -b. It is separate from the existing "no MD tags" message.

Why refuse rather than fit the rest: a self-transition is the only symptom of a stale tag that can be detected. The same tag can also name the wrong reference base at a real mismatch, or miss one entirely, and both look like ordinary evidence. Dropping the visible cases would leave a count built on tags already shown not to match the reads. An earlier revision of this PR fitted from the remaining mismatches; review caught it, and it now refuses the BAM.

read_bam_transition_report returns counts, masked and disagreement tallies together; read_bam_transitions and read_bam_transitions_masked are thin wrappers, so their tests are unchanged.

2. A population with no bases

Reproduced: 1 bp R1 reads with an R2 file of empty records failed with InvalidWeights([0.0]). With 2 bp R1 reads the same input was already refused clearly.

Cause: the population coverage check counted positions after the first. A 1 bp model has none, so the check could not fire, and an R2 with no seed reached the quality fit.

Fix: the check counts the seed. A population with no seed covers 0 bp and is refused as the R2 population covers 0 bp, but the model covers 1 bp …. This is the same rule the check already applied, now extended to position 1.

How much this matters: the crash needs a contrived input (every R1 read 1 bp, every R2 record empty), which real data does not produce. The change is kept because the check was miscounting in ordinary cases too. It assumed every population had a first base, so a population with no data was reported as covering 1 bp. With 2 bp R1 reads and an empty R2, the input was correctly refused, but the message said "covers 1 bp"; it now says 0 bp.

Single-file cases did not reach InvalidWeights, checked with the binary:

  • all 1 bp reads give a valid 1 bp model;
  • all-empty records are refused as "No bases found";
  • a mix fits from the 1 bp reads.

Evidence

  • Four new runner tests, each red before the fix for the predicted reason:
    • stale MD only: the error names the disagreement and the count;
    • mixed stale and genuine MD: refused, naming 3 stale and 4 consistent positions, and no model is written;
    • 1 bp R1 with an empty R2: refused, naming R2 at 0 bp;
    • must-not-fire: a 1 bp pair fits, with a separate R2 model.
  • Mutation checks, all killed:
    • counting stale MD positions again fails the stale-MD tests;
    • refusing only when no consistent mismatches remain (the earlier fit-the-rest behavior) fails the mixed test;
    • ignoring the seed in the coverage check fails the R2 refusal test.
  • Full workspace suite (1,095 tests), fmt --check and clippy -D warnings pass locally.

Not verified

  • Not run on a real BAM with stale MD tags. The fixtures are synthetic, as the issue asked: it is a constructed failure, and a real aligner writes consistent MD.

Closes #767.

🤖 Generated with Claude Code

Both paths the #765 audit traced to a bare InvalidWeights are reachable,
and both now fail, or fit, for a stated reason.

1. MD tags that disagree with SEQ. TransitionObserver counted an MD
   mismatch position without checking that the read base differs from
   the reference base, so a stale tag recorded a self-transition; the
   diagonal is zeroed when the matrix is built, and a row holding only
   those had no weight. Such positions are now left out and tallied.
   The runner warns with the count, and when they are the only evidence
   it says the MD tags disagree with the reads, how many positions, and
   to rewrite them with samtools calmd. A BAM with some stale positions
   now fits from its genuine mismatches instead of failing.

2. A population with no bases. The coverage check counted transition
   positions only, which a 1 bp model has none of, so it could not
   fire: 1 bp R1 reads with an all-empty R2 reached the quality fit
   with no seed and failed as InvalidWeights([0.0]). The check now
   counts the seed: a population with none covers 0 bp and is refused
   by name. A pair of 1 bp files still fits.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@joshfactorial joshfactorial mentioned this pull request Oct 5, 2026
9 of 35 tasks
joshfactorial and others added 2 commits October 4, 2026 20:58
Plain wording for comments that described populations whose reads stop
at different lengths as 'ragged'. Test and fixture names are unchanged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A self-transition is the only detectable symptom of a stale MD tag; the
same tag can mis-state the reference base at a real mismatch, and that
counts as ordinary evidence. Fitting from the remaining mismatches would
rest on tags already shown not to describe the reads, so any
disagreement is now a hard error naming both counts and calmd.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@joshfactorial
joshfactorial merged commit 21429e4 into develop Oct 5, 2026
7 checks passed
@joshfactorial
joshfactorial deleted the fix/767_zero_weight_distributions branch October 5, 2026 12:08
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.

1 participant