Repository navigation
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
🟡 Changes recommended
The 10x auto-detection path still hard-codes the multiome whitelist filename and can abort under set -euo pipefail if only the new gex_-prefixed arc whitelist is present, plus there are a couple of user-facing message/help text issues to correct.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR extends the 10x platform wrapper to optionally bypass chemistry auto-detection (--wl <chemistry>) and downgrade read-length sanity checks to warnings (--skip-length-checks), while also removing unmapped-read FASTQ generation (--outReadsUnmapped Fastx) across platforms and bumping the CLI version to 4.3.
Changes:
- Add
--wlchemistry override (10x only) and refactor 10x whitelist/geometry/metrics logic to share code paths. - Add
--skip-length-checks(10x only) to log warnings instead of aborting on unexpected R1/R2 lengths (with a new always-fatal UMI-less R1 guard). - Remove unmapped-read FASTQ output generation from all platform scripts; bump
STARSOLO_VERSIONto 4.3.
File summaries
| File | Description |
|---|---|
| README.md | Documents new 10x-only flags and how to use them. |
| lib/platform_strt.sh | Stops requesting unmapped reads from STAR. |
| lib/platform_smartseq.sh | Stops requesting unmapped reads from STAR. |
| lib/platform_rhapsody.sh | Stops requesting unmapped reads from STAR. |
| lib/platform_indrops.sh | Stops requesting unmapped reads from STAR. |
| lib/platform_dropseq.sh | Stops requesting unmapped reads from STAR. |
| lib/platform_10x.sh | Implements --wl, --skip-length-checks, refactors detection/metrics, and removes unmapped reads output. |
| bin/starsolo | Adds CLI parsing/guards for new flags and bumps version to 4.3. |
Review details
Suppressed comments (1)
lib/platform_10x.sh:101
- Multiome whitelist matching hard-codes 737K-arc-v1.txt. If a whitelist directory only contains the newer gex_737K-arc-v1.txt (which this PR now prefers for --wl), grep -f will error under set -euo pipefail and abort chemistry auto-detection. Make auto-detection choose whichever arc-v1 whitelist exists (and skip the scan if neither is present).
local NBC1 NBC2 NBC3 NBC4 NBC5 NBCA
NBC1=$(awk 'NR%4==2' test.R1.fastq | cut -c-14 | grep -F -f "$WL/737K-april-2014_rc.txt" | wc -l)
NBC2=$(awk 'NR%4==2' test.R1.fastq | cut -c-16 | grep -F -f "$WL/737K-august-2016.txt" | wc -l)
NBC3=$(awk 'NR%4==2' test.R1.fastq | cut -c-16 | grep -F -f "$WL/3M-february-2018.txt" | wc -l)
NBC4=$(awk 'NR%4==2' test.R1.fastq | cut -c-16 | grep -F -f "$WL/3M-3pgex-may-2023.txt" | wc -l)
NBC5=$(awk 'NR%4==2' test.R1.fastq | cut -c-16 | grep -F -f "$WL/3M-5pgex-jan-2023.txt" | wc -l)
NBCA=$(awk 'NR%4==2' test.R1.fastq | cut -c-16 | grep -F -f "$WL/737K-arc-v1.txt" | wc -l)
local BC=""
if (( NBC3 > 50000 )); then BC="$WL/3M-february-2018.txt"
elif (( NBC2 > 50000 )); then BC="$WL/737K-august-2016.txt"
elif (( NBCA > 50000 )); then BC="$WL/737K-arc-v1.txt"
elif (( NBC1 > 50000 )); then BC="$WL/737K-april-2014_rc.txt"
- Files reviewed: 7/8 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| local rl_info PAIRED CBLEN UMILEN | ||
| rl_info=$(_10x_check_read_lengths "$R1DIS" "$R1LEN" "$R2LEN" "$BC" "$WL") | ||
| local rl_info PAIRED | ||
| [[ $SKIPLEN == "True" ]] && log_warn "Read-length validation disabled (--skip-length-checks)." |
| --wl <chemistry> Use this chemistry instead of detecting it. Names | ||
| chemistry ids: |
Summary
Two new opt-in 10x flags, plus one behaviour change to output:
--wl <chemistry>(10x) — skip barcode-whitelist matching by naming the chemistry directly, using the same ids asinfer_10x_run.py.--skip-length-checks(10x) — downgrade the R1/R2 read-length sanity checks from fatal to warnings.--outReadsUnmapped Fastxremoved from all platforms.Commits
d3d9f2c--outReadsUnmapped Fastxoption from multiple platform scripts940b81bf05111fplatform_10x.sh9c55460Changes in detail
1.
--wl <chemistry>— skip 10x chemistry detection (10x only)When the chemistry is already known,
--wlselects the whitelist and CB/UMIgeometry directly and skips the six
grep -F -fwhitelist scans over thesubsampled reads.
Names deliberately match the chemistry ids emitted by
infer_10x_run.pyinnf-reprocessing-public-10x, so a value read out of that script's JSON report canbe passed straight through. Short aliases are accepted for interactive use, and
matching is case-insensitive.
--wlvaluegex_3pv1v1gex_3pv2_or_5pv1v2v2gex_3pv3_familyv3gex_3pv4_gemxv4gex_5pv3_gemx5pv3gex_multiome_arc_v1multiome,arcNotes:
gex_-prefixed whitelist and falls back to737K-arc-v1.txton older whitelist directories.starsolo 10xisgene-expression only.
specificity is still determined by test alignment, since it cannot be inferred
from the whitelist.
strand.txtreportsNAfor the per-whitelist match countsin this mode.
2.
--skip-length-checks— non-fatal read-length validation (10x only)The three pre-alignment sanity checks (variable-length R1, R1 < 24 bp, R2 < 40 bp)
become warnings instead of aborting. Implemented by swapping the handler
(
die→log_warn) rather than skipping the checks, so nothing goes silent.CB/UMI reconciliation against R1 still applies — a short R1 still trims the UMI
length with a warning. One new hard error was added there: an R1 shorter than the
cell barcode leaves no room for a UMI and would produce an invalid STAR command,
so that dies regardless of the flag. This path was previously unreachable, but
becomes reachable now that a short R1 no longer aborts.
3. Unmapped reads no longer generated
--outReadsUnmapped Fastxremoved from all six platform scripts (10x×2,smartseq,dropseq,rhapsody,indrops,strt).lib/common.shis unchanged: itspbzip2compression ofUnmapped.out.mate1/2is guarded by
[[ -s ... ]], so it simply no longer fires.pbzip2is retainedas a dependency. Legacy scripts under
scripts/legacy/are untouched.4. Internal refactor of
lib/platform_10x.shTo let the auto-detect and
--wlpaths share code:_10x_read_metrics— new; measures R1/R2 length and the R1 length distribution.Previously
_10x_determine_whitelistdid this alongside whitelist matching._10x_whitelist_geometry— new; the whitelist-file → CB/UMI map, extracted fromthe inline
casein_10x_check_read_lengths._10x_resolve_chemistry— new; chemistry name → whitelist + geometry._10x_check_read_lengths— now only validates lengths and reconciles CB/UMIagainst R1; takes CB/UMI as arguments instead of deriving them.
Compatibility
unchanged except that unmapped-read FASTQs are no longer produced.
Unmapped.out.mate1.bz2/mate2.bz2from a run directory will no longer find them. Nothing in this reporeads those files.
--wland--skip-length-checksare rejected with an explicit error onnon-10x platforms, where the whitelist is fixed.
Before merging
STARSOLO_VERSIONinbin/starsolofrom4.2to4.3(and the# Versionheader comment on line 5) — currently still4.2ondev.Testing
bash -nclean onbin/starsoloand all oflib/._10x_resolve_chemistry: all 6 ids + 8 aliases resolve to existing files in/nfs/cellgeni/STAR/whitelists; case-insensitivity confirmed; ATAC ids andunknown names produce the intended errors.
_10x_check_read_lengths: each of the three checks aborts by default andwarns-and-continues under
--skip-length-checks; the UMI-less R1 case stilldies with the flag set; normal v3 single-end (
16|12) and paired-end(
True|16|12) results unchanged; short-R1 trimming produces16|4as expected.--wl v3 --skip-length-checksparse together.
.github/workflows/test.yml) runs all platforms end-to-end on the testreference.
Not run end-to-end against production FASTQs.
Review focus
--wlname table vs.CHEMISTRIESininfer_10x_run_recommended.py— thesemust stay in sync for the nf-reprocessing handoff to work.
_10x_check_read_lengthsfor an R1 shorter than the cellbarcode — the only new failure mode in this PR. Previously unreachable (the
R1LEN < 24check caught it first); reachable now that--skip-length-checksdowngrades that check to a warning. Without the guard,
UMILENgoes negativeand STAR is called with e.g.
--soloUMIlen -2.