fix(test): the unmapped-reason test re-implemented the logic it was testing - #169
Open
BenjaminDEMAILLE wants to merge 3 commits into
Open
fix(test): the unmapped-reason test re-implemented the logic it was testing#169BenjaminDEMAILLE wants to merge 3 commits into
BenjaminDEMAILLE wants to merge 3 commits into
Conversation
This was referenced Jul 30, 2026
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>
BenjaminDEMAILLE
force-pushed
the
bd/filter-reasons-counters
branch
from
August 27, 2026 12:05
2e20e0d to
715bdcf
Compare
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
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.
A test that was not testing the shipped code, and the per-read allocation it was hiding.
The test
read_align.rshad this inside the test body: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
FilterReasonsisMaking 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 theunmapped_reasondecision, which is unchanged and now under test.Debugprints the non-zero reasons in a fixed order, which reads better than the map's arbitrary one.SpliceJunctionDbmoves toFxHashMapin the same spirit — it is queried per candidate junction while stitching, andcluster_seedsalready made this choice for its integer-keyed maps. Onlygetandinsertare 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:
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.