cli parity #147 with fixes - #216
Merged
Merged
Conversation
…1 knobs this PR covers Adds `tests/parameter_surface.rs` and the name list it checks against. The list is the column-1 names of STAR 2.7.11b's `parametersDefault`, minus the three non-user meta-parameters (`parametersFiles`, `sysShell`, `versionGenome`): 200 names, the public CLI surface only, no STAR source. The test asserts every one of those names is either recognised by clap or listed in `NOT_YET_ACCEPTED`, so the coverage figure is machine-checked rather than claimed, and surface drift fails a test the moment it happens instead of turning up in a bug report. It also asserts the reverse: a name listed as not accepted that has since been implemented must be removed from the list. Two supporting lists keep "accepted" from quietly meaning "implemented": - `ACCEPTED_BUT_INERT` names the 16 knobs that parse but change no output byte, each with the reason. A second test checks the CLI really does accept them, so the annotation cannot drift into being a lie. - `NOT_YET_ACCEPTED` names the 26 still missing, grouped by the theme that will bring them, so the remaining gap is legible rather than a bare number. This commit declares the 31 parameters in this PR's scope. Behaviour follows in the next commits; the ones that will stay inert are already annotated as such. Surface goes from 143/200 to 174/200 accepted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…or as STAR does Implements the behavioural half of this theme. SAM output: - `--outSAMmode None` routes to the existing null writer, so alignment still runs and SJ.out.tab, Log.final.out and the count matrices are still produced; only the records are dropped. `NoQS` strips quality strings through a small decorator around the boxed writer, rather than at each of the five record-building sites, so the clone is paid only when the flag is set. - `--outSAMheaderHD`, `--outSAMheaderPG` and `--outSAMheaderCommentFile` build the @hd, extra @pg and @co lines, with TAG:value fields validated rather than passed through. Splice junctions: - `--outSJtype None` suppresses SJ.out.tab. - `--outSJfilterReads Unique` counts only uniquely-mapping reads towards the outSJfilter* thresholds, so a junction supported solely by multimappers drops out. Read input: - `--readFilesPrefix` is applied during parsing, so every consumer sees final paths. - `--readQualityScoreBase` and `--outQSconversionAdd` become a single signed shift applied as the FASTQ is read, normalising to Phred+33 once. - `--readNameSeparator` cuts read names at the first separator. **One output change at default settings.** STAR's default `--readNameSeparator` is `/`, and STAR cuts there. rustar-aligner did not, so a read named `foo/1` was emitted as `foo/1` where STAR emits `foo`. Verified against native STAR 2.7.11b: STAR sim_0_YDR305C_mRNA_j567 origin/main sim_0_YDR305C_mRNA_j567/1 this branch sim_0_YDR305C_mRNA_j567 The unit test that asserted the old behaviour has been updated; it was encoding the divergence. Reads whose names contain no separator are unaffected, which is why the 1000-read differential set is byte-identical to origin/main. Loud rejections rather than silent acceptance: - `--outSAMfilter KeepOnlyAddedReferences` / `KeepAllAddedReferences` need align-time `--genomeFastaFiles` reference insertion, which this aligner does not do. - `--readFilesType SAM SE|PE` is refused rather than silently treating a SAM file as FASTQ. `--outSAMorder` is accepted at both values: `run_batch_pipeline` already consumes batches in input order, so `PairedKeepInputOrder` is satisfied. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Nine integration tests over the SAM/SJ/read-input knobs: accepted values, rejected values, `--readFilesPrefix` applied to both mates, negative `--outQSconversionAdd` surviving clap's hyphen handling, and the inert limit family parsing at STAR's defaults. They drive `Parameters` directly rather than spawning the binary: every knob here is decided at parse time or at a single output site, so a full alignment run would add minutes without adding coverage. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…owns #145 landed `--outSAMorder` and `--alignEndsType` while this branch was open. The duplicate `--outSAMorder` declaration and its validation are removed, along with the test that covered them, and `alignEndsType` moves out of `NOT_YET_ACCEPTED` since it is now implemented upstream. The parameter-surface test needed no other change, which is the point of it: it reports the new number by itself. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ixture The fixture's own header said it was STAR's parametersDefault names "minus the three non-user meta-params parametersFiles, sysShell, versionGenome". All three are settable on STAR's command line like any other parameter, so removing them did not describe a narrower surface, it hid three names rustar-aligner does not accept and shrank the denominator the coverage figure is quoted against. The fixture is now every column-1 name of parametersDefault, 203 of them, and the three sit in NOT_YET_ACCEPTED where the honest gap is recorded. Coverage reads 175/203 rather than 175/200. clap rejecting them is the right behaviour meanwhile, and worth keeping deliberately: silently accepting --parametersFiles would discard every parameter in the file the user passed.
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.
CLI and output parity from @BenjaminDEMAILLE — 31 STAR 2.7.11b parameters, and a test that turns the parameter surface into a machine-checked number. His commits are preserved with original authorship; my fixes follow as their own commits.
Closes #147
What's in it
tests/parameter_surface.rschecks every parameter name STAR 2.7.11b accepts against what clap recognises — all 203 column-1 names fromparametersDefault, interface only. Two supporting lists stop "accepted" meaning "implemented":ACCEPTED_BUT_INERT(knobs that parse but change no output byte, each with a reason) andNOT_YET_ACCEPTED(the real gap, grouped by theme). The test fails in both directions — an unrecognised name that is not declared missing, and a name declared missing that has since been implemented.Flags that now do something:
--outSAMmode None/NoQS,--outSJtype None,--outSJfilterReads Unique,--outSAMheaderHD/PG/CommentFile,--readFilesPrefix,--readQualityScoreBase,--outQSconversionAdd,--readNameSeparator.Changes made while merging
--outSJfilterReads Uniquewas filtering in the wrong place. It discounted multi-mapping reads at output time, zeroing column 8 while leaving column 9 (max spliced overhang) built from every read. Against STAR on 10k yeast reads, junctionII:332876came out1 0 57where STAR gives1 0 14— same counts, an overhang no unique read supports.STAR gates the recording instead:
ReadAlign::recordSJskips the read entirely unlessoutSJfilterReads=="All" || nTrO==1, so a multimapper contributes nothing — not its counts, not its overhang.SpliceJunctionStatsnow carries the flag andrecord_junctionreturns early for a non-unique read, which makes the output-time discount redundant.Result: under the flag,
SJ.out.tabdiffers from STAR on 3 lines instead of 5, and those 3 are exactly the ones that already differ at defaults — so the flag no longer adds divergence of its own.Two implemented parameters were listed as inert.
runRNGseedfeedsper_read_seed, which seeds the shuffle under--outMultimapperOrder Random: the same 10k reads at--runRNGseed 777versus424242give 1444 differing records. Its stated reason ("seeded per read, not from a global RNG") described the mechanism and drew the wrong conclusion — the per-read seed is derived from the global one.limitBAMsortRAMis enforced inio/bam.rsand ends the run:limitBAMsortRAM=1000 bytes exceeded: estimated 3889200 bytes for 9723 records.Both are still accepted, so they simply leave the list. I checked the rest of it too:
limitGenomeGenerateRAMlooks used but only logs "accepted but not enforced", so it is genuinely inert and stays.The list's inert half is not machine-checked —
inert_parameters_are_actually_acceptedverifies each name is accepted, never that it is unread. That gap is what let these two sit there, and it is now noted in the doc comment so the next entry is added deliberately.Three stale
NOT_YET_ACCEPTEDentries.genomeChrSetMitochondrial,soloCellReadStatsandsoloClusterCBfilelanded in #200 and #212. The surface test caught them itself on its first run against currentmain, which is the drift it exists to catch.The default output change had no test that could fail. See below.
Corrections to the PR description
Two figures in #147's description no longer hold, both because the tree moved under it:
ACCEPTED_BUT_INERTholds 22 knobs. It held 16; after removing the two implemented ones it holds 14.Both are now printed by
report_coveragerather than counted by hand:Worth noting the description also opens with a self-correction, retracting an earlier revision that had trimmed three names out of the fixture and quietly shrunk the denominator.
The one output change at default settings
STAR's default
--readNameSeparatoris/and STAR cuts there; this codebase did not.ERR12389696.12376817mainERR12389696.12376817/1ERR12389696.12376817Verified against STAR on reads whose QNAMEs carry a
/1: every QNAME matches STAR's.test_read_name_separator_cuts_the_qname_and_is_configurableis an added test for--readNameSeparator -. Stops a hardcoded/passing as flag-driven behaviour. Confirmed it fails when the separator list is cleared.Validation
Defaults byte-identical to
main. Records compared with headers excluded, since the@PG CL:line necessarily differs by output prefix:mainSJ.out.tabat defaultsAgainst STAR 2.7.11b, unchanged from the recorded baselines:
Reproduce the differentials with the dataset from CONTRIBUTING:
Note for whichever merges second
Four flags in
NOT_YET_ACCEPTEDhere (--genomeType,--genomeTransformOutput,--genomeSuffixLengthMax,--sjdbInsertSave) are implemented on #109 and #160. The surface test will fail on whichever lands second until they are moved out of that list — by design.