Skip to content

fix(test): the unmapped-reason test re-implemented the logic it was testing - #169

Open
BenjaminDEMAILLE wants to merge 5 commits into
scverse:mainfrom
BenjaminDEMAILLE:bd/filter-reasons-counters
Open

fix(test): the unmapped-reason test re-implemented the logic it was testing#169
BenjaminDEMAILLE wants to merge 5 commits into
scverse:mainfrom
BenjaminDEMAILLE:bd/filter-reasons-counters

Conversation

@BenjaminDEMAILLE

Copy link
Copy Markdown
Contributor

A test that was not testing the shipped code, and the per-read allocation it was hiding.

The test

read_align.rs had this inside the test body:

let classify = |reasons: &[&str]| -> UnmappedReason {
    let filter_reasons: std::collections::HashMap<&str, i32> =
        reasons.iter().map(|k| (*k, 1)).collect();
    let has_mismatch = filter_reasons.contains_key("mismatch_max")
        || filter_reasons.contains_key("mismatch_rate");
    let has_other = filter_reasons
        .keys()
        .any(|k| *k != "mismatch_max" && *k != "mismatch_rate");
    ...
};

It rebuilds the decision rather than calling it. The assertions below it are fine, but they check a copy of the logic that lives in the test, so the test would pass unchanged if the production code were deleted. It is asserting that the test author can write an if.

It now drives FilterReasons, the same counters the aligner uses, so changing that decision fails the test.

What FilterReasons is

Making the test call the real thing meant giving the real thing a name. The filter loop kept its counts in a HashMap<&str, i32> built per read and incremented per filtered transcript: a heap allocation on every read and a string hash on every increment, for something whose only consumers are two debug logs and one boolean decision.

It is now a [u32; 9] indexed by constant. The consumers ask only whether a count is non-zero, so nothing here can reach the output except through the unmapped_reason decision, which is unchanged and now under test. Debug prints the non-zero reasons in a fixed order, which reads better than the map's arbitrary one.

SpliceJunctionDb moves to FxHashMap in the same spirit — it is queried per candidate junction while stitching, and cluster_seeds already made this choice for its integer-keyed maps. Only get and insert are ever called on it and it is never iterated, so the hasher cannot reach the output.

Performance: no

Neither change is measurable end to end, and I would rather say so than dress this up as a perf PR. Six interleaved rounds on 2M reads at 16 threads:

median
before 20.70 s
after 20.48 s

Direction mixed across the six, inside the run-to-run spread, and the machine sat at 72-85% idle rather than the 88% the bench script gates on. Call it unmeasured.

They are here because the test was not testing anything and because both changes remove work that has no reason to exist, not because of a number.

For scale on why one allocation per read is not measurable: the align path does about 1 000 heap allocations per read (measured in #168). This removes one of them.

Verification

Output-neutral: SAM byte-identical on 200 000 real reads (ERR12389696, yeast) against the parent commit, whole file including header.

Gate: 561 lib + 26 integration tests, cargo clippy --all-targets -- -D warnings, cargo fmt --check, all green.

BenjaminDEMAILLE and others added 5 commits July 30, 2026 20:07
`detect_splice_motif` and `find_best_junction_position` were the two
largest self-time entries in a sampled 2M-read run, together about 45%
of the on-CPU samples.

The scan moves the junction one base per iteration, so the four bases
that decide the motif (the intron's first two and last two) overlap the
previous position in two of four places. Carrying them in a
`MotifWindow` and sliding turns four genome reads per position into two.
Whether the motif branch runs at all is decided once before the loop
rather than per iteration, since `del` does not change across the scan.

An out-of-range position becomes a sentinel that no motif arm matches,
which is what `get_base` returning `None` already meant.

Output-neutral: SAM byte-identical to the parent commit on 200k real
reads. Interleaved A/B at 16 threads on 2M reads, machine at 87-95% CPU
idle: 22.94s median before, 22.29s after, the same direction in all four
rounds. About 2.8%.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The four-base motif decision compiled to a chain of comparisons whose
outcome is data-dependent and close to unpredictable, and the junction
scan pays it at every position. A 256-entry table indexed by the four
bases packed two bits each replaces it with one load.

`examples/jrbench.rs` measures the scan in isolation: 8.5 ns per
iteration before, 5.2 ns after, a 38% cut, with identical results.

That example exists because the earlier attempts on this path were guesses
about whether the loop was memory-bound, and two of them were wrong. It
answers the question by construction, holding the iteration count and the
instruction mix fixed and changing only how far the acceptor sits from the
donor:

    del          ns/iter
    64           8.42
    1024         8.47
    16384        8.52
    262144       8.41
    4194304      8.62
    67108864     8.56

Flat from 64 bases to 67 million, so the acceptor distance costs nothing
and the loop is not stalled on that stream. Which is why skipping the
acceptor read behind a donor-pair test measured *slower*: it traded a
prefetchable access for an unpredictable branch. The branch was the cost
all along, and this removes it.

`the_motif_table_agrees_with_the_original_match_on_every_input` checks
the table against the match it replaces over every combination of
`A`/`C`/`G`/`T`, `N`, the chromosome-boundary byte and the out-of-range
sentinel, so the inputs that no match arm covered are covered explicitly.

Output-neutral: SAM byte-identical on 200k real reads.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The test at `read_align.rs` built its own `HashMap<&str, i32>` and
re-implemented the `has_mismatch` / `has_other` decision inside the test
body, then asserted on its own copy. It would have passed unchanged if
the shipped code had been deleted.

It now drives `FilterReasons`, which is the same set of counters the
aligner uses, so a change to that decision fails the test.

`FilterReasons` replaces the `HashMap<&str, i32>` the filter loop used:
a fixed `[u32; 9]` indexed by constant. That was a heap allocation per
read and a string hash per filtered transcript, for something whose only
consumers ask whether a count is non-zero (two debug logs and the
`unmapped_reason` decision), so nothing here can reach the output except
through that decision. Its `Debug` prints only the non-zero reasons, in
a fixed order, which is more readable than the map's arbitrary one.

`SpliceJunctionDb` moves to `FxHashMap` in the same spirit, matching what
`cluster_seeds` already does for its integer-keyed maps. Only `get` and
`insert` are called on it and it is never iterated.

Honest about the performance: **neither change is measurable end to end.**
Six interleaved rounds on 2M reads gave medians 20.70 s against 20.48 s
with the direction mixed, which is inside the run-to-run spread. They are
here because they remove work and because the test was not testing
anything, not because they show up on a clock.

Output-neutral: SAM byte-identical on 200k real reads.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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