Skip to content

Commit 792f569

Browse files
rob-pclaude
andcommitted
fix: forward --barcode-length, expose --with-position, retire two no-ops
Three findings from a pass over the option surface, comparing what simpleaf accepts against what the child tools actually take. `atac process --barcode-length` was parsed, documented with its own default of 16, and never forwarded; piscem spells it `--bclen`. Because both defaults are 16 this was invisible, but a non-16bp ATAC chemistry got a 16 bp barcode extracted -- and since the length is written into the RAD tags and read back by generate-permit-list, sort and deduplicate, every stage agreed with the wrong value. This is the same failure as `--thr`, so both are now pinned by tests that assert the built command line, on the ATAC and RNA sides alike. `--use-chr` and `--check-kmer-orphan` are hidden and marked deprecated. piscem accepts them and warns "not supported by piscem-rs and will be ignored", so simpleaf was advertising behaviour no supported piscem implements. `--with-position` is now exposed on `quant`. libradicl has a positional record type and alevin-fry already dispatches on it in generate-permit-list, collate and quant, so the flag needed no downstream counterpart -- only piscem had to be told. Verified end to end on the 66.6 M-read pbmc set: the pipeline runs through to a matrix identical to the non-positional run (0 differing entries of 5,573,896), with the RAD growing 2.6 GB -> 4.5 GB. Deliberately not offered on `multiplex-quant`: there is no multi-barcode positional record type, and get_record_type_from_prelude checks multi-barcode first, so such a RAD would be read back as plain multi-barcode. Also documents why the chemistry registry is served from `dev` rather than the release branch, replacing a "TODO: change to main repo when we are ready" that read as an oversight rather than a decision. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VhyNvEWUgY8ALVacdJHxVJ
1 parent 54fc50c commit 792f569

10 files changed

Lines changed: 213 additions & 16 deletions

File tree

CHANGELOG.md

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,13 @@
2727
`multiplex-quant`, and `atac process`.
2828
* `--tmp-dir` and `--ram-limit-gib` are forwarded to `piscem build` from both
2929
index commands.
30+
* `--with-position` on `quant`, forwarded to piscem. Records each mapped read's
31+
position in the RAD file; alevin-fry detects the positional record type from
32+
the RAD header and adapts on its own, so no downstream option changes. The
33+
RAD is larger (2.6 GB -> 4.5 GB on a 66.6 M-read set) and counts are
34+
unchanged. Not offered on `multiplex-quant`: there is no multi-barcode
35+
positional record type, and the multi-barcode layout wins when the record
36+
type is resolved.
3037
* `--small-thresh` on `quant` and `multiplex-quant`, forwarded to alevin-fry.
3138
Cells below it are resolved by alevin-fry's tiny-cell fast path, which
3239
applies `cr-like` semantics regardless of `--resolution`; `--small-thresh 0`
@@ -41,6 +48,15 @@
4148
### Bug fixes
4249

4350
* `atac process --thr` was parsed and then never forwarded to piscem.
51+
* `atac process --barcode-length` was likewise never forwarded (piscem spells it
52+
`--bclen`). Both defaults are 16, so it silently did nothing when set: any
53+
non-16bp ATAC chemistry got a 16 bp barcode extracted, and because the length
54+
is written into the RAD tags and read back by every later stage, the whole
55+
pipeline agreed with itself and nothing looked wrong.
56+
* `atac process --use-chr` and `--check-kmer-orphan` are hidden and documented
57+
as deprecated no-ops. piscem >= 0.22 accepts them for backward compatibility
58+
and warns that they are ignored, so they were advertising behaviour that no
59+
supported piscem implements.
4460
* `multiplex-quant` did not validate its `--geometry`, so a typo surfaced as a
4561
mapping failure partway through a run rather than as an error up front.
4662
* `multiplex-quant` logged its `map-sc` invocation as "piscem map-scrna cmd".

docs-site/src/content/docs/quant-command.md

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -122,6 +122,14 @@ Piscem Mapping Options:
122122
--struct-constraints
123123
Enable structural constraints when mapping
124124

125+
--with-position
126+
Record the position of each mapped read in the RAD file.
127+
128+
alevin-fry detects the positional record type from the RAD header and adapts
129+
automatically, so no downstream option needs to change. The RAD file is larger. Not
130+
available for `multiplex-quant`: there is no multi-barcode positional record type, and the
131+
multi-barcode layout takes precedence when the record type is resolved.
132+
125133
--ignore-ambig-hits
126134
Skip checking of the equivalence classes of k-mers that were too ambiguous to be otherwise
127135
considered (passing this flag can speed up mapping slightly, but may reduce specificity)

src/atac/commands.rs

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -369,8 +369,10 @@ pub struct ProcessOpts {
369369
#[arg(long, help_heading = "Advanced Options")]
370370
pub no_poison: bool,
371371

372-
/// use chromosomes as color
373-
#[arg(long, help_heading = "Advanced Options")]
372+
/// Deprecated no-op. piscem-rs does not implement this; piscem >= 0.22
373+
/// accepts the flag for backward compatibility and warns that it is
374+
/// ignored. Retained so existing command lines keep working.
375+
#[arg(long, help_heading = "Advanced Options", hide = true)]
374376
pub use_chr: bool,
375377

376378
/// threshold to be considered for pseudoalignment
@@ -389,10 +391,10 @@ pub struct ProcessOpts {
389391
#[arg(long, help_heading = "Advanced Options")]
390392
pub no_tn5_shift: bool,
391393

392-
/// Check if any mapping kmer exist for a mate which is not mapped,
393-
/// but there exists mapping for the other read. If set to true and a
394-
/// mapping kmer exists, then the pair would not be mapped
395-
#[arg(long, help_heading = "Advanced Options")]
394+
/// Deprecated no-op. piscem-rs does not implement this; piscem >= 0.22
395+
/// accepts the flag for backward compatibility and warns that it is
396+
/// ignored. Retained so existing command lines keep working.
397+
#[arg(long, help_heading = "Advanced Options", hide = true)]
396398
pub check_kmer_orphan: bool,
397399

398400
/// determines the maximum cardinality equivalence class (number of

src/atac/process.rs

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,15 @@ fn push_advanced_piscem_options(
7979
// used its own default. piscem's `map-sc-atac` takes it, so pass it on.
8080
piscem_map_cmd.arg("--thr").arg(opts.thr.to_string());
8181

82+
// Same story as `--thr`: `--barcode-length` was parsed and documented (with
83+
// its own default of 16) and then never forwarded, so piscem fell back to
84+
// its own default -- identical at 16, which is why this went unnoticed, but
85+
// any user who set it got their value silently ignored. piscem spells it
86+
// `--bclen`.
87+
piscem_map_cmd
88+
.arg("--bclen")
89+
.arg(opts.barcode_length.to_string());
90+
8291
Ok(())
8392
}
8493

@@ -706,4 +715,75 @@ mod tests {
706715
err
707716
);
708717
}
718+
719+
fn args_of(cmd: &std::process::Command) -> Vec<String> {
720+
cmd.get_args()
721+
.map(|a| a.to_string_lossy().into_owned())
722+
.collect()
723+
}
724+
725+
/// Every scalar option that `atac process` accepts and hands to piscem must
726+
/// actually appear on the child command line.
727+
///
728+
/// Both `--thr` and `--barcode-length` were parsed, documented with their
729+
/// own defaults, and then never forwarded. Neither failure was visible from
730+
/// the outside: piscem's defaults happened to match, so the options simply
731+
/// did nothing when set. This pins the pass-through so the next one is a
732+
/// test failure rather than a silent no-op.
733+
#[test]
734+
fn advanced_piscem_options_reach_the_child_command() {
735+
let mut opts = base_process_opts();
736+
opts.thr = 0.55;
737+
opts.barcode_length = 20;
738+
opts.max_ec_card = 1234;
739+
opts.max_hit_occ = 77;
740+
opts.max_hit_occ_recover = 888;
741+
opts.max_read_occ = 99;
742+
743+
let mut cmd = std::process::Command::new("echo");
744+
push_advanced_piscem_options(&mut cmd, &opts).expect("should build args");
745+
let args = args_of(&cmd);
746+
747+
for (flag, value) in [
748+
("--thr", "0.55"),
749+
("--bclen", "20"),
750+
("--max-ec-card", "1234"),
751+
("--max-hit-occ", "77"),
752+
("--max-hit-occ-recover", "888"),
753+
("--max-read-occ", "99"),
754+
] {
755+
let pos = args.iter().position(|a| a == flag).unwrap_or_else(|| {
756+
panic!("{} was never forwarded to piscem; args: {:?}", flag, args)
757+
});
758+
assert_eq!(
759+
args.get(pos + 1).map(String::as_str),
760+
Some(value),
761+
"{} was forwarded with the wrong value; args: {:?}",
762+
flag,
763+
args
764+
);
765+
}
766+
}
767+
768+
/// The boolean passthroughs are emitted only when set.
769+
#[test]
770+
fn boolean_piscem_options_are_conditional() {
771+
let mut opts = base_process_opts();
772+
opts.ignore_ambig_hits = false;
773+
opts.no_poison = false;
774+
775+
let mut cmd = std::process::Command::new("echo");
776+
push_advanced_piscem_options(&mut cmd, &opts).expect("should build args");
777+
let args = args_of(&cmd);
778+
assert!(!args.iter().any(|a| a == "--ignore-ambig-hits"));
779+
assert!(!args.iter().any(|a| a == "--no-poison"));
780+
781+
opts.ignore_ambig_hits = true;
782+
opts.no_poison = true;
783+
let mut cmd = std::process::Command::new("echo");
784+
push_advanced_piscem_options(&mut cmd, &opts).expect("should build args");
785+
let args = args_of(&cmd);
786+
assert!(args.iter().any(|a| a == "--ignore-ambig-hits"));
787+
assert!(args.iter().any(|a| a == "--no-poison"));
788+
}
709789
}

src/simpleaf_commands.rs

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -221,6 +221,16 @@ pub struct MapQuantOpts {
221221
#[arg(long, help_heading = "Piscem Mapping Options")]
222222
pub struct_constraints: bool,
223223

224+
/// Record the position of each mapped read in the RAD file.
225+
///
226+
/// alevin-fry detects the positional record type from the RAD header and
227+
/// adapts automatically, so no downstream option needs to change. The RAD
228+
/// file is larger. Not available for `multiplex-quant`: there is no
229+
/// multi-barcode positional record type, and the multi-barcode layout takes
230+
/// precedence when the record type is resolved.
231+
#[arg(long, help_heading = "Piscem Mapping Options")]
232+
pub with_position: bool,
233+
224234
/// Skip checking of the equivalence classes of k-mers that were too ambiguous to be otherwise
225235
/// considered (passing this flag can speed up mapping slightly, but may reduce specificity)
226236
#[arg(

src/simpleaf_commands/quant.rs

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -215,6 +215,13 @@ fn push_advanced_piscem_options(
215215
piscem_quant_cmd.arg("--struct-constraints");
216216
}
217217

218+
// alevin-fry resolves the positional record type from the RAD header
219+
// (KnownRecordType::RnaShortPos) and switches record types itself, so this
220+
// needs no counterpart on the generate-permit-list / collate / quant calls.
221+
if opts.with_position {
222+
piscem_quant_cmd.arg("--with-position");
223+
}
224+
218225
piscem_quant_cmd
219226
.arg("--max-hit-occ")
220227
.arg(format!("{}", opts.max_hit_occ));
@@ -733,4 +740,69 @@ mod tests {
733740
err
734741
);
735742
}
743+
744+
/// The piscem passthroughs on the RNA path must reach the child command.
745+
///
746+
/// Same rationale as the ATAC-side test: `--thr` and `--barcode-length`
747+
/// both rotted there by being parsed and never forwarded, which is
748+
/// invisible from outside when the child's default happens to match.
749+
#[test]
750+
fn advanced_piscem_options_reach_the_child_command() {
751+
let opts = parse_quant_opts(&[
752+
"quant",
753+
"-c",
754+
"10xv3",
755+
"-o",
756+
"/tmp/out",
757+
"-r",
758+
"cr-like",
759+
"--knee",
760+
"--map-dir",
761+
"/tmp/mapped",
762+
"--with-position",
763+
"--struct-constraints",
764+
"--max-hit-occ",
765+
"77",
766+
]);
767+
let mut cmd = std::process::Command::new("echo");
768+
push_advanced_piscem_options(&mut cmd, &opts).expect("should build args");
769+
let args: Vec<String> = cmd
770+
.get_args()
771+
.map(|a| a.to_string_lossy().into_owned())
772+
.collect();
773+
774+
assert!(
775+
args.iter().any(|a| a == "--with-position"),
776+
"--with-position was never forwarded; args: {:?}",
777+
args
778+
);
779+
assert!(args.iter().any(|a| a == "--struct-constraints"));
780+
let pos = args
781+
.iter()
782+
.position(|a| a == "--max-hit-occ")
783+
.expect("flag");
784+
assert_eq!(args.get(pos + 1).map(String::as_str), Some("77"));
785+
786+
// and the booleans stay off when not requested
787+
let opts = parse_quant_opts(&[
788+
"quant",
789+
"-c",
790+
"10xv3",
791+
"-o",
792+
"/tmp/out",
793+
"-r",
794+
"cr-like",
795+
"--knee",
796+
"--map-dir",
797+
"/tmp/mapped",
798+
]);
799+
let mut cmd = std::process::Command::new("echo");
800+
push_advanced_piscem_options(&mut cmd, &opts).expect("should build args");
801+
let args: Vec<String> = cmd
802+
.get_args()
803+
.map(|a| a.to_string_lossy().into_owned())
804+
.collect();
805+
assert!(!args.iter().any(|a| a == "--with-position"));
806+
assert!(!args.iter().any(|a| a == "--struct-constraints"));
807+
}
736808
}

src/utils/chem_utils.rs

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,8 +10,6 @@ use std::path::Path;
1010
use strum::EnumIter;
1111
use strum::IntoEnumIterator;
1212

13-
// TODO: Change to main repo when we are ready
14-
1513
pub(crate) type CustomChemistryMap = HashMap<String, CustomChemistry>;
1614

1715
static GEOMETRY_KEY: &str = "geometry";

src/utils/constants.rs

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,17 @@
66
pub(crate) static CUSTOM_CHEMISTRIES_PATH: &str = "custom_chemistries.json";
77

88
pub(crate) static CHEMISTRIES_PATH: &str = "chemistries.json";
9+
10+
/// The chemistry registry is served from `dev`, deliberately, and not from the
11+
/// release branch. Chemistry definitions are data rather than code: a new or
12+
/// corrected geometry needs to reach users who are already on a released
13+
/// simpleaf, without waiting for the next release. `simpleaf chemistry refresh`
14+
/// therefore always fetches the newest registry.
15+
///
16+
/// The trade-off is that a released binary's chemistry definitions are whatever
17+
/// `dev` holds at the time of the fetch, not what shipped with it. Keep `dev`'s
18+
/// copy of `resources/chemistries.json` in sync when landing registry changes
19+
/// on `main`, or a released simpleaf will not see them.
920
pub(crate) static CHEMISTRIES_URL: &str =
1021
"https://raw.githubusercontent.com/COMBINE-lab/simpleaf/dev/resources/chemistries.json";
1122

tests/snapshots/cli-help/simpleaf_atac_process___help.txt

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -99,9 +99,6 @@ Advanced Options:
9999
the mapping results will be identical to those obtained as if no poison table was added to
100100
the index
101101

102-
--use-chr
103-
use chromosomes as color
104-
105102
--thr <THR>
106103
threshold to be considered for pseudoalignment
107104

@@ -120,11 +117,6 @@ Advanced Options:
120117
--no-tn5-shift
121118
do not apply Tn5 shift to mapped positions
122119

123-
--check-kmer-orphan
124-
Check if any mapping kmer exist for a mate which is not mapped, but there exists mapping
125-
for the other read. If set to true and a mapping kmer exists, then the pair would not be
126-
mapped
127-
128120
--max-ec-card <MAX_EC_CARD>
129121
determines the maximum cardinality equivalence class (number of (txp, orientation status)
130122
pairs) to examine (cannot be used with --ignore-ambig-hits)

tests/snapshots/cli-help/simpleaf_quant___help.txt

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,14 @@ Piscem Mapping Options:
3838
--struct-constraints
3939
Enable structural constraints when mapping
4040

41+
--with-position
42+
Record the position of each mapped read in the RAD file.
43+
44+
alevin-fry detects the positional record type from the RAD header and adapts
45+
automatically, so no downstream option needs to change. The RAD file is larger. Not
46+
available for `multiplex-quant`: there is no multi-barcode positional record type, and the
47+
multi-barcode layout takes precedence when the record type is resolved.
48+
4149
--ignore-ambig-hits
4250
Skip checking of the equivalence classes of k-mers that were too ambiguous to be otherwise
4351
considered (passing this flag can speed up mapping slightly, but may reduce specificity)

0 commit comments

Comments
 (0)