Skip to content

CLI + output parity: SAM/SJ/read-input knobs and the STAR limit surface - #147

Open
BenjaminDEMAILLE wants to merge 4 commits into
scverse:mainfrom
BenjaminDEMAILLE:bd/cli-output-parity
Open

CLI + output parity: SAM/SJ/read-input knobs and the STAR limit surface#147
BenjaminDEMAILLE wants to merge 4 commits into
scverse:mainfrom
BenjaminDEMAILLE:bd/cli-output-parity

Conversation

@BenjaminDEMAILLE

@BenjaminDEMAILLE BenjaminDEMAILLE commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

CLI and output parity: 31 STAR 2.7.11b parameters, and a test that makes the parameter surface a machine-checked number.

What changed

tests/parameter_surface.rs checks every user-facing parameter name STAR 2.7.11b accepts against what clap recognises. The name list is the column-1 names of STAR's parametersDefault, minus the three non-user meta-parameters (parametersFiles, sysShell, versionGenome): 200 names, interface only, no STAR source.

Two supporting lists stop "accepted" from meaning "implemented":

  • ACCEPTED_BUT_INERT — 22 knobs that parse but change no output byte, each with its reason. A second test asserts the CLI does accept them, so the annotation cannot drift.
  • NOT_YET_ACCEPTED — the 26 still missing, grouped by the theme that will bring them.

The test fails in both directions: an unrecognised name that is not declared missing fails, and a name still listed as missing that has since been implemented also fails.

143/200 → 174/200 accepted.

Flags that do something:

Flag Behaviour
--outSAMmode None routes to the null writer; SJ.out.tab, Log.final.out and counts are still produced, only records are dropped
--outSAMmode NoQS strips quality strings
--outSJtype None suppresses SJ.out.tab
--outSJfilterReads Unique only unique reads count towards outSJfilter*, so a junction supported solely by multimappers drops out
--outSAMheaderHD / PG / CommentFile build @hd, extra @pg and @co, with TAG:value validated rather than passed through
--readFilesPrefix applied during parsing, so every consumer sees final paths
--readQualityScoreBase, --outQSconversionAdd one signed shift applied as the FASTQ is read, normalising to Phred+33 once
--readNameSeparator cuts read names at the first separator

Why

Parameter coverage was a claim in prose. A test makes it a number that cannot drift, and the grouped NOT_YET_ACCEPTED list makes the remaining gap legible rather than a bare count.

NoQS is a decorator around the boxed writer rather than a branch at each of the five record-building sites, so the clone is paid only when the flag is set.

One output change at default settings

STAR's default --readNameSeparator is / and STAR cuts there; this codebase did not. On a read named sim_0_YDR305C_mRNA_j567/1, 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

Verification

Gate green: lib + integration tests, clippy -D warnings, cargo fmt --check, MSRV 1.89. New integration tests cover each implemented flag; the surface test covers the rest.

Bookkeeping note: four flags in NOT_YET_ACCEPTED here (--genomeType, --genomeTransformOutput, --genomeSuffixLengthMax, --sjdbInsertSave) are implemented on #109. Whichever merges second moves them out of the list in the same commit.

🤖 Generated with Claude Code

BenjaminDEMAILLE and others added 4 commits July 28, 2026 22:21
…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>
…it now owns

scverse#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>
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