fix: make dupRadar and Qualimap outputs reproducible run-to-run - #142
Open
BenjaminDEMAILLE wants to merge 1 commit into
Open
fix: make dupRadar and Qualimap outputs reproducible run-to-run#142BenjaminDEMAILLE wants to merge 1 commit into
BenjaminDEMAILLE wants to merge 1 commit into
Conversation
Fixes seqeralabs#141. Three outputs differed between two runs of the same binary on the same input. In each case a HashMap was collected into a Vec and sorted with a comparator that has ties; `sort_by` is stable, so tied elements kept HashMap iteration order, which `RandomState` randomises per process. - `qualimap/output.rs`: junction motifs sorted by percentage, then truncated to the top 11. Many 4-mers tie, so which ones appear and in what order varied. Tie-break on the motif. - `qualimap/report.rs`: the same motif list in the HTML report. Same fix. - `dupradar/plots.rs`: the density scatter deduplicates points per pixel into a HashMap, then sorts by density so dense points draw last. Points of equal density came out in a different sequence each run, changing the SVG element order and the PNG bytes. Keep the pixel coordinate alongside the point and tie-break on it. Also fixes the same pattern in `compute_bias()`, which had a worse consequence: the transcript list is sorted by mean coverage and truncated to the top 1000, so ties at that boundary decided which transcripts fed the 5'/3' bias calculation. Tie-break on the transcript's flat index, assigned in GTF order. `flat_idx` is now used rather than dead code. Upstream Qualimap orders these by Java HashMap iteration, i.e. arbitrarily, so a deterministic tie-break is no less faithful than the current behaviour. Verified with three consecutive runs on a 4M-read BAM: `rnaseq_qc_results.txt`, the scatter `.svg` and `.png` are now byte-identical across runs. The contents are unchanged from before — sorting the old and new files gives identical multisets, only the order of tied entries moved. `qualimapReport.html` still differs between runs by exactly two lines: the "Analysis date" timestamp and its copy in the footer. That is intentional report metadata and matches upstream Qualimap. `cargo test --release` passes (200 + 12 + 18 + 2). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Fixes #141.
What
Adds a deterministic tie-breaker to four sorts whose input order came from a
HashMap.slice::sort_byis stable, so tied elements keep their input order. When that input is aHashMapiterated into aVec, the order is randomised per process byRandomState, and it reached the output files.qualimap/output.rsjunction motifsqualimap/report.rsjunction motifsdupradar/plots.rsscatter dedup<circle>draw order in the SVG, and therefore the PNG bytesqualimap/output.rscompute_biastruncate(1000), i.e. the reported bias valuesThe first three are cosmetic (no number changes). The fourth is the one that can actually move a reported metric; I did not catch it firing in practice, but it is the same bug and worth closing.
Tie-breakers chosen: motif string, pixel coordinate, and the transcript's flat index (assigned in GTF order).
flat_idxwas carrying#[allow(dead_code)]; it now has a use.Faithfulness to upstream
Upstream Qualimap orders these lists by Java
HashMapiteration, i.e. arbitrarily. A deterministic tie-break is no less faithful than what RustQC does today, and it is reproducible.Verification
Three consecutive runs of the patched binary on the same 4M-read BAM:
Content is unchanged from
main— sorting the old and new files yields identical multisets, only the order of tied entries moved. Every other output file is byte-identical tomain(modulo the output directory path that some headers embed).qualimapReport.htmlstill differs between runs by exactly two lines:That is the report timestamp and its footer copy — intentional metadata, matching upstream Qualimap. Everything else in the report is now stable.
cargo test --releasepasses (200 + 12 + 18 + 2).cargo clippy -- -D warningsclean.🤖 Generated with Claude Code
Part of #143.