Repository navigation
fix(extract): match fgbio strict read-name UMI extraction (EXT-01/03/04) - #489
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughUracil now complements to adenine, while read-name UMI extraction uses fallible strict normalization with reverse-complement handling, delimiter conversion, upper-casing, and ChangesDNA complement U/u support
Read-name UMI extraction strictness
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant RecordBuilder
participant extract_read_name_and_umi
participant reverse_complement
RecordBuilder->>extract_read_name_and_umi: read name bytes
alt r-prefixed segment
extract_read_name_and_umi->>reverse_complement: segment bytes
reverse_complement-->>extract_read_name_and_umi: complemented bytes
end
extract_read_name_and_umi->>extract_read_name_and_umi: replace + with -, uppercase, validate ACGTN-
alt invalid character
extract_read_name_and_umi-->>RecordBuilder: error
else valid
extract_read_name_and_umi-->>RecordBuilder: normalized UMI
end
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #489 +/- ##
==========================================
- Coverage 91.14% 91.08% -0.06%
==========================================
Files 78 78
Lines 51606 51726 +120
==========================================
+ Hits 47034 47116 +82
- Misses 4572 4610 +38 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
0b4a291 to
a9e4a9a
Compare
|
Follow-up (amended into the same commit): made the Verified against fgbio 4.1.0:
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/commands/extract.rs`:
- Around line 2517-2531: The current
`test_extract_read_name_and_umi_rejects_illegal_chars` uses a manual loop over
multiple illegal inputs, which hides the failing case and does not follow the
Rust test convention. Convert this into a parameterized `#[rstest]` test with
separate cases for each illegal UMI input, keeping the assertion against
`Extract::extract_read_name_and_umi` in the same test logic so each input is
reported independently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: dc7766dc-44e9-41f1-9f07-5f2a57ceacd1
📒 Files selected for processing (2)
crates/fgumi-dna/src/dna.rssrc/lib/commands/extract.rs
a9e4a9a to
50f36c4
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
50f36c4 to
f165260
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/commands/extract.rs`:
- Around line 2539-2546: The test for Extract::extract_read_name_and_umi
currently loops over multiple headers, which makes it hard to see which input
failed; convert it to parameterized #[rstest] cases like
test_extract_read_name_and_umi_rejects_illegal_chars. Keep the same assertions
for extract_read_name_and_umi, but split the inputs (`@sample_1`, `@foo.2`, `@bar`:1)
into separate rstest cases so each regression reports the exact header that
broke.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c3f688a8-3b50-46d8-bb2d-67b03a3f318d
📒 Files selected for processing (2)
crates/fgumi-dna/src/dna.rssrc/lib/commands/extract.rs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/commands/extract.rs`:
- Around line 2539-2546: The test for Extract::extract_read_name_and_umi
currently loops over multiple headers, which makes it hard to see which input
failed; convert it to parameterized #[rstest] cases like
test_extract_read_name_and_umi_rejects_illegal_chars. Keep the same assertions
for extract_read_name_and_umi, but split the inputs (`@sample_1`, `@foo.2`, `@bar`:1)
into separate rstest cases so each regression reports the exact header that
broke.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c3f688a8-3b50-46d8-bb2d-67b03a3f318d
📒 Files selected for processing (2)
crates/fgumi-dna/src/dna.rssrc/lib/commands/extract.rs
🛑 Comments failed to post (1)
src/lib/commands/extract.rs (1)
2539-2546: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Loop-over-inputs test hides which input regressed; convert to
#[rstest]cases. Same convention already applied totest_extract_read_name_and_umi_rejects_illegal_chars.♻️ Convert to rstest
- #[test] - fn test_extract_read_name_and_umi_does_not_strip_dot_or_underscore_suffix() { - // Only `/` is a read-number separator; `.`/`_`/`:` may be part of a real - // name and must be preserved (unlike the broader validation helper). - for header in [b"`@sample_1`".as_slice(), b"`@foo.2`".as_slice(), b"`@bar`:1".as_slice()] { - let (name, _umi) = Extract::extract_read_name_and_umi(header, false).unwrap(); - assert_eq!(name, header[1..].to_vec(), "should not strip: {header:?}"); - } - } + // Only `/` is a read-number separator; `.`/`_`/`:` may be part of a real + // name and must be preserved (unlike the broader validation helper). + #[rstest] + #[case::underscore(b"`@sample_1`".as_slice())] + #[case::dot(b"`@foo.2`".as_slice())] + #[case::colon(b"`@bar`:1".as_slice())] + fn test_extract_read_name_and_umi_does_not_strip_dot_or_underscore_suffix(#[case] header: &[u8]) { + let (name, _umi) = Extract::extract_read_name_and_umi(header, false).unwrap(); + assert_eq!(name, header[1..].to_vec(), "should not strip: {header:?}"); + }As per coding guidelines: "Use
rstestfor parameterized tests in Rust test files".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/commands/extract.rs` around lines 2539 - 2546, The test for Extract::extract_read_name_and_umi currently loops over multiple headers, which makes it hard to see which input failed; convert it to parameterized #[rstest] cases like test_extract_read_name_and_umi_rejects_illegal_chars. Keep the same assertions for extract_read_name_and_umi, but split the inputs (`@sample_1`, `@foo.2`, `@bar`:1) into separate rstest cases so each regression reports the exact header that broke.Source: Coding guidelines
`fgumi extract --extract-umis-from-read-names` mirrors fgbio's `FastqToBam -n`,
which calls `Umis.extractUmisFromReadName(name, strict=true)`. That path
upper-cases the extracted UMI, reverse-complements `r`-prefixed segments, and
throws on any character outside `ACGTN-`. fgumi previously wrote the last
read-name field to `RX` verbatim: no upper-casing, no `r`-revcomp, and it
silently accepted illegal UMI characters.
- EXT-03: upper-case the read-name UMI.
- EXT-04: reverse-complement `r`-prefixed segments (for a `+`-delimited dual
UMI, only the prefixed segment is reverse-complemented); reuse the shared
`fgumi_dna::dna::reverse_complement` helper.
- EXT-01: reject (error) a UMI containing any character outside `ACGTN-`,
matching fgbio strict mode, and surface the offending read name.
Also teach `fgumi_dna`'s complement to map RNA uracil `U`/`u` -> `A`/`a`
(fgbio `Sequences.complement`), so an `r`-prefixed UMI containing `U` is
reverse-complemented identically to fgbio (`rAAU` -> `ATT`). A `U` in a non-`r`
UMI is not reverse-complemented and is rejected by both tools.
EXT-02 (lenient `>=8`-field / last-field extraction) is intentionally kept, so
the field-count logic is unchanged. `is_valid_umi_char` mirrors fgbio's
`Umis.isValidUmiCharacter` (`ACGTN-`) and is deliberately distinct from
`fgumi_umi::validate_umi`, which implements the more permissive
`GroupReadsByUmi` counting rule.
fgbio<->fgumi parity (ext-01-03-04-readname-umi.sh, fgbio 4.1.0):
before: RX `acgtn` / `rGGTTAA` / `acgt-rGGTTAA` vs fgbio
`ACGTN` / `TTAACC` / `ACGT-TTAACC` (DIFFER); illegal `ACGTXY`
accepted (rc=0) vs fgbio rejected (rc=1)
after: RX tags MATCH fgbio exactly (incl. `rAAU` -> `ATT`); illegal
`ACGTXY` rejected (rc=1)
Refs: EXT-01, EXT-03, EXT-04 (fgbio behavioral parity tracker).
f165260 to
9879c95
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
`strip_read_suffix` canonicalizes read names before checking that
paired/interleaved FASTQ streams are in sync (that the reads at the same
position across streams share a name). Its old rule was too lenient:
after stripping a trailing space-separated comment it also stripped a
trailing `separator + digit` for separator in {`/`, `.`, `_`, `:`} and
digit in {`1`, `2`}. That collapses a genuinely-mismatched pair such as
`read_1` (stream 0) and `read_2` (stream 1) to the same base name
`read`, so two different reads are silently accepted as "in sync".
Narrow the rule to match fgbio's `FastqSource` read-name
canonicalization (`com/fulcrumgenomics/fastq/FastqSource.scala`), which
strips only a trailing `/` followed by a single ASCII digit (0-9). The
space-comment strip is unchanged; `.`/`_`/`:` separators and multi-digit
runs (`read/12`) are now preserved. This is the same rule PR #489
established for the extract path, applied here to the shared
FASTQ-sync helper.
All three production callers are paired-FASTQ name-sync validation and
benefit uniformly with no call-site changes: `FastqGrouper`
(`grouper.rs`), `zip_fastq`, and `parse_zip_fastq`.
Behavior tradeoff: narrowing correctly rejects mismatched `read_1` /
`read_2` streams, but it ALSO now rejects non-standard paired FASTQs
that legitimately use `.1`/`.2`/`_1`/`_2`/`:1`/`:2` read-number
suffixes. This matches fgbio (which rejects them too), so it is correct
fgbio parity, but it is a real behavior change reviewers should note.
Future cleanup: `extract.rs` still carries its own broad
`strip_read_suffix_extract`; consolidating both onto the narrow shared
helper is a follow-up left out of this focused fix.
This branch was previously deployed
1 participant
Summary
fgumi extract --extract-umis-from-read-namesis fgumi's port of fgbio'sFastqToBam -n, which callsUmis.extractUmisFromReadName(name, strict=true). fgbio's strict path upper-cases the extracted UMI, reverse-complementsr-prefixed segments, and throws on any character outsideACGTN-. fgumi previously wrote the last read-name field toRXverbatim — no upper-casing, nor-revcomp, and it silently accepted illegal UMI characters.This closes three findings from the fgbio behavioral-parity tracker:
Umis.scala:111-117). Case-sensitive UMIs otherwise mis-group vs fgbio.r-prefixed segments (Umis.scala:102-115,reverseComplementPrefixedUmisdefaults true, asFastqToBamuses it). For a+-delimited dual UMI only the prefixed segment is reverse-complemented. Reuses the sharedfgumi_dna::dna::reverse_complementhelper rather than a new copy.ACGTN-, matching fgbio strict mode (Umis.scala:121-123,UmisTest.scala:110-114). The surfaced error names the offending read.Out of scope by decision: EXT-02 — fgumi's lenient
>=8-field / last-field extraction (which also supports demultiplexers that append a sample index, producing 9+ fields) is intentionally kept (tracker Decisions Q2). The field-count logic is unchanged; only the transform + validation of the extracted field now matches fgbio.The new
is_valid_umi_charmirrors fgbio'sUmis.isValidUmiCharacter(ACGTN-) and is deliberately distinct fromfgumi_umi::validate_umi, which implements the more permissiveGroupReadsByUmicounting rule (only rejects upper-caseN).fgbio ↔ fgumi parity evidence
Adversarial fixture (8-field read names, so fgbio strict and fgumi both select the same last field), run against fgbio 4.1.0:
FastqToBam -n…:acgtnACGTNacgtnACGTN…:rGGTTAATTAACCrGGTTAATTAACC…:acgt+rGGTTAAACGT-TTAACCacgt-rGGTTAAACGT-TTAACC…:ACGTXY(illegal)Before:
RESULT: DIFFER. After: RX tags match fgbio exactly and the illegal UMI is rejected like fgbio:Testing
r-prefix revcomp (single + dual-UMI), illegal-char rejection (ACGTXY,ACGT-CCKC,CCKC-ACGTperUmisTest.scala), a valid-UMI boundary, and lowercase+-delimited normalization.cargo ci-fmt,cargo ci-lint,cargo ci-test(2221 passed) all clean.Refs: EXT-01, EXT-03, EXT-04 (fgbio behavioral parity tracker; PR 2 of the burn-down).
Summary by CodeRabbit
New Features
U/uas complements toA/a, including in reverse-complement behavior.r-segment reverse-complementing, and dual-UMI separator translation.Bug Fixes
ACGTN-set.Tests & Documentation
U/ucomplement behavior and stricter UMI semantics.