Repository navigation
fix(extract): truncate read names at any whitespace and validate UMI field count - #1040
Conversation
…field count strip_read_suffix cut the FASTQ header only at the first space. A TAB-separated comment, such as the tags `samtools fastq -T RX,BC` writes (`name\tRX:Z:ACGT\tBC:Z:TTTT`), stayed in the QNAME. With --extract-umis-from-read-names, RX was then taken from the last `:` field of that comment, which is the sample barcode. The name is now cut at the first whitespace character as defined by Java's Character.isWhitespace over ASCII, matching fgbio FastqSource.scala:111 (`header.indexWhere(_.isWhitespace)`). The change applies to both extract and the paired-FASTQ sync validation, which share the helper. A header with no name before its first whitespace (`@\tRX:Z:ACGT`, `@ 1:N:0:ACGT`) or whose name is only a `/1` or `/2` suffix used to be written with an empty QNAME, which the SAM spec does not allow. Mates with different names that both stripped to "" also passed the pair-sync check. It is now an error quoting the header, with or without -n. The check lives in a new canonical_read_name helper used by the pipeline zip step, FastqGrouper and the extract record builder, so every FASTQ reader path (including de-interleaved input) enforces it. fgbio accepts these headers and writes the empty name. With --extract-umis-from-read-names, a name with 6 or fewer `:`-separated fields used to be written silently without a UMI. It is now rejected with a message adapted from fgbio's strict-mode one (Umis.scala:95): "Trying to extract UMI from read with <n> colon-separated fields; expected at least 7 (7 = no UMI, 8 or more = UMI in the last field). Check the read-name format, or omit --extract-umis-from-read-names if the read names carry no UMI. Read name: <name>". fgbio's "(7-8 expected)" would be wrong here because fgumi accepts 9 or more fields. A 7-field name still has no UMI and an 8-field name takes its last field. A name with 9 or more fields also takes its last field, a deliberate divergence from fgbio, which throws. An empty last field still gives no UMI from the name (EXT3-09), where fgbio extracts an empty one. Corrects comments that said fgbio's extractor always returns the last field. That is true only of its non-strict mode, while FastqToBam uses strict mode. Documents the field-count rules, the empty-last-field case and the whitespace cut in the extract --help and the fgbio migration guide, and corrects the copy-umi module doc and the deinterleave docs and test comment that described the old rules. Changes output for: extract with FASTQ headers whose comment is separated by a TAB or another non-space whitespace character (shorter QNAME; with -n, RX now comes from the name rather than the comment), extract -n with read names of 6 or fewer fields (now an error rather than reads written without RX), and extract with a header that has no read name (now an error rather than a record with an empty QNAME).
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (7)
Included review availability: This review used your included allowance. 3 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 4 reviews per hour.
WalkthroughFASTQ read-name handling now strips whitespace-delimited comments and rejects empty canonical names. UMI extraction applies field-count rules to canonical names. The grouper and pipeline zipper compare records using those names. ChangesRead-name handling
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Suggested labels: Merge Risk: ⚪ Minimal · up to The described read-name and UMI rules are consistent across extraction and paired-read matching. No specific issue remains to resolve before normal merge checks. 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
Comment |
|
@coderabbitai pause |
✅ Action performedReviews paused. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1040 +/- ##
==========================================
- Coverage 96.47% 96.45% -0.03%
==========================================
Files 299 299
Lines 152214 152284 +70
==========================================
+ Hits 146854 146888 +34
- Misses 5360 5396 +36 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
|
This branch was successfully deployed
1 participant
Summary
strip_read_suffixcut a FASTQ header only at the first space. A comment separated by a TAB, such as the tagssamtools fastq -T RX,BCwrites (name\tRX:Z:ACGT\tBC:Z:TTTT), stayed in the QNAME. With--extract-umis-from-read-names,RXwas then taken from the last:field of that comment, which is the sample barcode. The name is now cut at the first whitespace character, using Java'sCharacter.isWhitespaceover ASCII (space, TAB, LF, VT, FF, CR,0x1C-0x1F), as fgbio'sFastqSourcedoes. Extract and the paired-FASTQ mate-name sync checks in the FASTQ zip step andFastqGrouperall use this helper, so they change together. For the sync checks this only accepts more input: any pair that matched before still matches.A header with no name before its first whitespace (
@\tRX:Z:ACGT,@ 1:N:0:ACGT), or whose name is only a/1or/2suffix, used to be written with an empty QNAME, which the SAM spec does not allow. Mates with different names that both stripped to""also passed the sync check. This is now an error that quotes the header, with or without-n. The guard is a newcanonical_read_namehelper used by the zip step,FastqGrouperand the extract record builder, so every FASTQ reader path enforces it, including de-interleaved input. The parsers still keep the raw header as is. fgbio accepts these headers:FastqSource.scala:111-120yields"",FastqSource.zipped(:72) compares"" == "",FastqToBam.scala:196sets the name, and htsjdk'sSAMRecord.setReadNameandBAMRecordCodec.encode(:176) write it unchecked. This is a deliberate divergence.With
--extract-umis-from-read-names, a name with 6 or fewer:-separated fields used to be written without a UMI and without any warning. It is now rejected with a message adapted from fgbio's strict-mode one,Trying to extract UMI from read with <n> colon-separated fields; expected at least 7 (7 = no UMI, 8 or more = UMI in the last field). Check the read-name format, or omit --extract-umis-from-read-names if the read names carry no UMI. Read name: <name>, where<name>is the read name without its comment. The hint comes before the name so it cannot be misread as part of it. A 7-field name still has no UMI, and an 8-field name takes the last field as its UMI. Two behaviors are unchanged and differ from fgbio on purpose. A name with 9 or more fields takes its last field, where fgbio throws. An empty last field gives no UMI from the name. fgbio extracts an empty UMI, so it writes an emptyRX(or-<read UMI>withMsegments), where fgumi writes noRX(or the read UMI alone).Several doc comments said fgbio's extractor always returns the last field. That is only true of its non-strict mode (
CopyUmiFromReadName).FastqToBamuses strict mode. Those comments are fixed, as are thecopy-umimodule doc and the deinterleave module doc, function doc and test comment, which described the old rules or implied a UMI could come from the comment. The field-count rules, the empty-last-field case and the whitespace cut are now documented inextract --help, theExtractrustdoc, and a new "UMIs From Read Names" section in the fgbio migration guide.Changes output for:
extractwhen a FASTQ header's comment is separated by a TAB or another whitespace character that is not a space. The QNAME is shorter, and with-ntheRXcomes from the name and not from the comment.extract -nwhen a read name has 6 or fewer fields. The run now fails with an error, where before the reads were written withoutRX.extractwhen a header has no read name before its first whitespace, or only a/1or/2suffix. The run now fails with an error, where before the record was written with an empty QNAME.extract(andFastqGrouper) when mates have the same name but different TAB-separated comments. These pairs used to fail with a read name mismatch. They are now accepted.fgbio parity (e51a661)
FastqSource.scala:111splits the header atheader.indexWhere(_.isWhitespace)and then strips a trailing/and one digit.Umis.scala:91-95, strict mode: 6 colons givesNone, 7 colons gives the last field, and any other count throwsIllegalArgumentExceptionwithTrying to extract UMI from read with <n> parts (7-8 expected): <name>.FastqToBam.scala:188calls it withstrict=trueon the first read's name.FastqToBamTest.scala:358("extract UMIs from read names when requested"). This test is ported with TAB comments added to the headers, and it runs single-end and paired, with and without threads.UmisTest.scala:47("return None if the read name has only 7 parts in strict mode") andUmisTest.scala:51("throw an exception in strict mode if the read has too many or too few segments"). The1:2:3:4:5:6case is ported. Its1:2:3:4:5:6:7:8:ACGTcase is the 9+-field divergence described above.Tests
test_strip_read_suffix: new cases for TAB comments, TAB combined with a space and with/1, both ends of each delimiter range, and bytes that must stay in the name (!,A,~, NUL, 0x08, 0x0E, 0x1B, DEL, 0x80, 0x85, 0xA0, 0xFF).test_is_read_name_delimiter_matches_java_is_whitespace_over_bytes: pins the delimiter set over all 256 bytes.canonical_read_nameunit tests: a leading TAB, a leading space,/1and an empty header are errors.zip_step_and_grouper_classify_read_names_identically: guard-set parity. One case table runs through the zip step (record-by-record parse) andFastqGrouper(batched parse, with each stream split mid-record), including mismatched mates that used to collapse to"".test_extract_read_name_and_umi_truncates_name_at_tab: the name and the UMI, with and without-n.test_extract_read_name_and_umi_errors_on_six_or_fewer_fields: asserts the full error message for 6, 5, 2 and 1 fields, and for a 6-field name followed by a space or TAB comment.-n(no error) and for an empty 8th field (no UMI).test_extract_umis_from_read_names), run single-end and paired (with R1 and R2 comments that differ), the 6-field error, and the empty-name error for a leading TAB, a leading space, mismatched paired mates and interleaved mates, with and without-n. All run single-threaded and with 2 threads.Mutation-checked:
FASTQ read name mismatch.Checks
cargo ci-fmt,cargo ci-lint,cargo ci-doc,cargo ci-doctest,cargo ci-tag-literalsandcargo ci-test(11294 passed, 31 skipped) all pass.