feat: allow variable length UMIs - #1148
Open
TimD1 wants to merge 5 commits into
Open
Conversation
TimD1
commented
Mar 24, 2026
TimD1
force-pushed
the
557_td_allow-varable-length-umis
branch
from
March 24, 2026 20:53
01b9d61 to
cc4ee24
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1148 +/- ##
==========================================
+ Coverage 95.97% 96.02% +0.05%
==========================================
Files 132 132
Lines 8040 8168 +128
Branches 562 612 +50
==========================================
+ Hits 7716 7843 +127
- Misses 324 325 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
📝 WalkthroughWalkthroughCorrectUmis was changed to accept fixed UMI sets of multiple lengths, grouping fixed UMIs by length and matching BAM UMIs only against same-length groups; metrics and unmatched placeholders are tracked per length. GroupReadsByUmi adds a 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/scala/com/fulcrumgenomics/umi/GroupReadsByUmi.scala (1)
851-862:⚠️ Potential issue | 🟠 MajorTruncation is still using raw string width.
Line 862 slices characters, but the new contract on Lines 562-565 defines UMI length by bases and excludes separators.
ACT-GGGwith--min-umi-length=6is already 6bp; this code turns it intoACT-GG, which can merge families incorrectly when--truncate=true.Possible fix
+ private def truncateUmi(umi: Umi, targetLength: Int): Umi = { + val out = new StringBuilder() + var keptBases = 0 + umi.foreach { c => + val isUmiBase = SequenceUtil.isUpperACGTN(c.toUpper.toByte) + if (!isUmiBase || keptBases < targetLength) out.append(c) + if (isUmiBase) keptBases += 1 + } + out.toString + } + private def truncateUmis(umis: Seq[Umi]): Seq[Umi] = this.minUmiLength match { case None => umis case Some(length) => this.assigner match { case _: PairedUmiAssigner => throw new IllegalStateException("Cannot used the paired umi assigner when min-umi-length is defined.") case _ => val minLength = umis.map(_.length).min require(length <= minLength, s"Bug: UMI found that had shorter length than expected ($minLength < $length)") - umis.map(_.substring(0, length)) + umis.map(umi => truncateUmi(umi, length)) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/scala/com/fulcrumgenomics/umi/GroupReadsByUmi.scala` around lines 851 - 862, The truncateUmis method currently measures and slices UMIs by raw string length and substring (using Umi.length and .substring), which incorrectly counts separators (e.g. '-' ) and corrupts base-count truncation; update truncateUmis to compute lengths in bases (excluding separators) and truncate each Umi by base count while preserving separators/structure instead of using raw substring. Specifically, when matching minUmiLength/Some(length) and for non-PairedUmiAssigner cases, compute minLength as the minimum number of bases in each Umi (not raw string chars) and map each Umi to a new Umi truncated to the first N bases (preserving separators), e.g. by adding/using a Umi method that returns the first N bases or by iterating bases of each Umi and reconstructing the truncated Umi.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/scala/com/fulcrumgenomics/umi/GroupReadsByUmi.scala`:
- Around line 562-565: The help text incorrectly implies only truncation is
unsupported for paired UMIs, but the code (Strategy.Paired rejection when
--min-umi-length is set) actually disallows any min-umi-length filtering in
paired mode; update the help strings in GroupReadsByUmi.scala (the
UMI-length/--min-umi-length/--truncate help paragraphs and the similar block
around lines ~597-601) to explicitly state that --min-umi-length is not
supported for Strategy.Paired (i.e., paired UMIs cannot use min-umi-length or
truncation), so the documentation matches the runtime check that rejects
Strategy.Paired with --min-umi-length.
---
Outside diff comments:
In `@src/main/scala/com/fulcrumgenomics/umi/GroupReadsByUmi.scala`:
- Around line 851-862: The truncateUmis method currently measures and slices
UMIs by raw string length and substring (using Umi.length and .substring), which
incorrectly counts separators (e.g. '-' ) and corrupts base-count truncation;
update truncateUmis to compute lengths in bases (excluding separators) and
truncate each Umi by base count while preserving separators/structure instead of
using raw substring. Specifically, when matching minUmiLength/Some(length) and
for non-PairedUmiAssigner cases, compute minLength as the minimum number of
bases in each Umi (not raw string chars) and map each Umi to a new Umi truncated
to the first N bases (preserving separators), e.g. by adding/using a Umi method
that returns the first N bases or by iterating bases of each Umi and
reconstructing the truncated Umi.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e31c5897-da08-45f5-8074-f251655d9ee8
📒 Files selected for processing (4)
src/main/scala/com/fulcrumgenomics/umi/CorrectUmis.scalasrc/main/scala/com/fulcrumgenomics/umi/GroupReadsByUmi.scalasrc/test/scala/com/fulcrumgenomics/umi/CorrectUmisTest.scalasrc/test/scala/com/fulcrumgenomics/umi/GroupReadsByUmiTest.scala
nh13
requested changes
Mar 28, 2026
nh13
left a comment
nh13
left a comment
Member
There was a problem hiding this comment.
Can you also fix up any coderabbitai suggestions?
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/main/scala/com/fulcrumgenomics/umi/CorrectUmis.scala (1)
260-266:⚠️ Potential issue | 🟠 MajorStill guard the zero-denominator cases in metric finalization.
The new
numReal == 0check helps, but Line 265 can still divide by0when every read is rejected, and Line 266 can still divide by0when only unmatched buckets accumulate counts. That will writeNaN/Infinityinto the metrics file.Proposed fix
val sortedMetrics = umiMetrics.values.toSeq.sortBy(_.umi) val totalWithUnmatched = sortedMetrics.map(_.total_matches).sum.toDouble val numReal = sortedMetrics.size - unmatchedUmiByLength.size val meanWithoutUnmatched = if (numReal == 0) 0d else sortedMetrics.filter(m => m.umi != unmatchedUmiByLength(m.umi.length)).map(_.total_matches).sum / numReal.toDouble sortedMetrics.foreach { m => - m.fraction_of_matches = m.total_matches / totalWithUnmatched - m.representation = m.total_matches / meanWithoutUnmatched + m.fraction_of_matches = if (totalWithUnmatched == 0d) 0d else m.total_matches / totalWithUnmatched + m.representation = if (meanWithoutUnmatched == 0d) 0d else m.total_matches / meanWithoutUnmatched }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/scala/com/fulcrumgenomics/umi/CorrectUmis.scala` around lines 260 - 266, The finalization can still divide by zero: compute totalWithUnmatched and meanWithoutUnmatched as before but when populating each UMI metric (in the loop over sortedMetrics) guard the two divisions by checking denominators; if totalWithUnmatched == 0 set m.fraction_of_matches = 0.0, and if meanWithoutUnmatched == 0 set m.representation = 0.0 (otherwise compute as m.total_matches / denominator). Keep using the existing symbols (sortedMetrics, totalWithUnmatched, meanWithoutUnmatched, unmatchedUmiByLength, m.total_matches, m.fraction_of_matches, m.representation) so no other logic changes are required.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/scala/com/fulcrumgenomics/umi/GroupReadsByUmi.scala`:
- Around line 607-608: The validation currently allows zero or negative values
for minUmiLength which breaks truncation logic; update the require checks around
minUmiLength/truncate in GroupReadsByUmi (the require guarding Strategy.Paired
and the require(!truncate || minUmiLength.isDefined)) to also reject
non-positive values by asserting minUmiLength.forall(_ > 0) (and where checking
definedness ensure minUmiLength.exists(_ > 0)), so that --min-umi-length must be
> 0 when provided and truncate cannot be used without a positive minUmiLength.
- Around line 817-835: In the non-paired branch of the rawToId computation (the
assigner match fallback that calls this.assigner.assign/_umis), replace
u.split("-") with u.split("-", -1) so that trailing empty UMI segments are
preserved (e.g., "ACGT-" remains distinct from "ACGT"); this ensures groupBy(u
=> u.split("-", -1).map(_.length).toSeq) produces correct length buckets for
AdjacencyUmiAssigner and prevents mixing raw UMI lengths.
---
Duplicate comments:
In `@src/main/scala/com/fulcrumgenomics/umi/CorrectUmis.scala`:
- Around line 260-266: The finalization can still divide by zero: compute
totalWithUnmatched and meanWithoutUnmatched as before but when populating each
UMI metric (in the loop over sortedMetrics) guard the two divisions by checking
denominators; if totalWithUnmatched == 0 set m.fraction_of_matches = 0.0, and if
meanWithoutUnmatched == 0 set m.representation = 0.0 (otherwise compute as
m.total_matches / denominator). Keep using the existing symbols (sortedMetrics,
totalWithUnmatched, meanWithoutUnmatched, unmatchedUmiByLength, m.total_matches,
m.fraction_of_matches, m.representation) so no other logic changes are required.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b6a41b99-aa67-45ef-b8bd-47a81b2c5984
📒 Files selected for processing (4)
src/main/scala/com/fulcrumgenomics/umi/CorrectUmis.scalasrc/main/scala/com/fulcrumgenomics/umi/GroupReadsByUmi.scalasrc/test/scala/com/fulcrumgenomics/umi/CorrectUmisTest.scalasrc/test/scala/com/fulcrumgenomics/umi/GroupReadsByUmiTest.scala
✅ Files skipped from review due to trivial changes (1)
- src/test/scala/com/fulcrumgenomics/umi/GroupReadsByUmiTest.scala
🚧 Files skipped from review as they are similar to previous changes (1)
- src/test/scala/com/fulcrumgenomics/umi/CorrectUmisTest.scala
Comment on lines
+817
to
+835
| val rawToId = this.assigner match { | ||
| case paired: PairedUmiAssigner => | ||
| umis | ||
| .groupBy { u => | ||
| val segs = u.split("-") | ||
| val aLen = segs.find(_.startsWith(paired.lowerReadUmiPrefix + ":")).map(_.stripPrefix(paired.lowerReadUmiPrefix + ":").length).getOrElse(throw new IllegalStateException(s"UMI segment missing expected prefix '${paired.lowerReadUmiPrefix}' in: $u")) | ||
| val bLen = segs.find(_.startsWith(paired.higherReadUmiPrefix + ":")).map(_.stripPrefix(paired.higherReadUmiPrefix + ":").length).getOrElse(throw new IllegalStateException(s"UMI segment missing expected prefix '${paired.higherReadUmiPrefix}' in: $u")) | ||
| (aLen, bLen) | ||
| } | ||
| .values | ||
| .flatMap { umisByLength => paired.assign(umisByLength) } | ||
| .toMap | ||
| case _ => | ||
| umis | ||
| .groupBy(u => u.split("-").map(_.length).toSeq) | ||
| .values | ||
| .flatMap { _umis => this.assigner.assign(_umis) } | ||
| .toMap | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Scala/Java, does String.split("-")drop trailing empty fields, and doessplit("-", -1) preserve them?
💡 Result:
Yes.
- Java / Scala (
java.lang.String):str.split("-")is defined asstr.split("-", 0), and with limit = 0 Java discards trailing empty strings (so"a-b-".split("-")yieldsArray("a","b")). [1] str.split("-", -1)uses a negative limit, meaning the pattern is applied as many times as possible and trailing empty strings are preserved (so"a-b-".split("-", -1)yieldsArray("a","b","")). [1]
In Scala StringOps, split(separator: Char) likewise excludes empty trailing substrings and is implemented in terms of Java’s String.split. [2]
Sources: [1] (docs.oracle.com), [2] (scala-lang.org)
Citations:
- 1: https://docs.oracle.com/en/java/javase/12/docs/api/java.base/java/lang/String.html
- 2: https://www.scala-lang.org/api/2.13.9/scala/collection/StringOps.html
🏁 Script executed:
# First, locate and check the exact code at line 831
rg -n "split\(\"-\"\)" src/main/scala/com/fulcrumgenomics/umi/GroupReadsByUmi.scala -A 2 -B 2Repository: fulcrumgenomics/fgbio
Length of output: 890
🏁 Script executed:
# Search for CorrectUmis file and its split usage
fd -i correctumis src/main/scala --type fRepository: fulcrumgenomics/fgbio
Length of output: 121
🏁 Script executed:
# Check if CorrectUmis uses split("-", -1)
rg -n "split\(\"-\"" src/main/scala/com/fulcrumgenomics/umi/ -lRepository: fulcrumgenomics/fgbio
Length of output: 318
🏁 Script executed:
# Check CorrectUmis.scala for split("-", -1) usage
rg -n "split\(" src/main/scala/com/fulcrumgenomics/umi/CorrectUmis.scala -B 1 -A 1Repository: fulcrumgenomics/fgbio
Length of output: 308
Preserve trailing empty UMI segments in non-paired case.
Line 831 uses split("-"), which drops trailing empty segments. A tag like ACGT- then collapses into the same bucket as ACGT, causing mixed raw string lengths to trip AdjacencyUmiAssigner's equal-length precondition. CorrectUmis already uses split("-", -1) for the same tag shape.
Proposed fix
case _ =>
umis
- .groupBy(u => u.split("-").map(_.length).toSeq)
+ .groupBy(u => u.split("-", -1).map(_.length).toSeq)
.values
.flatMap { _umis => this.assigner.assign(_umis) }
.toMap📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| val rawToId = this.assigner match { | |
| case paired: PairedUmiAssigner => | |
| umis | |
| .groupBy { u => | |
| val segs = u.split("-") | |
| val aLen = segs.find(_.startsWith(paired.lowerReadUmiPrefix + ":")).map(_.stripPrefix(paired.lowerReadUmiPrefix + ":").length).getOrElse(throw new IllegalStateException(s"UMI segment missing expected prefix '${paired.lowerReadUmiPrefix}' in: $u")) | |
| val bLen = segs.find(_.startsWith(paired.higherReadUmiPrefix + ":")).map(_.stripPrefix(paired.higherReadUmiPrefix + ":").length).getOrElse(throw new IllegalStateException(s"UMI segment missing expected prefix '${paired.higherReadUmiPrefix}' in: $u")) | |
| (aLen, bLen) | |
| } | |
| .values | |
| .flatMap { umisByLength => paired.assign(umisByLength) } | |
| .toMap | |
| case _ => | |
| umis | |
| .groupBy(u => u.split("-").map(_.length).toSeq) | |
| .values | |
| .flatMap { _umis => this.assigner.assign(_umis) } | |
| .toMap | |
| } | |
| val rawToId = this.assigner match { | |
| case paired: PairedUmiAssigner => | |
| umis | |
| .groupBy { u => | |
| val segs = u.split("-") | |
| val aLen = segs.find(_.startsWith(paired.lowerReadUmiPrefix + ":")).map(_.stripPrefix(paired.lowerReadUmiPrefix + ":").length).getOrElse(throw new IllegalStateException(s"UMI segment missing expected prefix '${paired.lowerReadUmiPrefix}' in: $u")) | |
| val bLen = segs.find(_.startsWith(paired.higherReadUmiPrefix + ":")).map(_.stripPrefix(paired.higherReadUmiPrefix + ":").length).getOrElse(throw new IllegalStateException(s"UMI segment missing expected prefix '${paired.higherReadUmiPrefix}' in: $u")) | |
| (aLen, bLen) | |
| } | |
| .values | |
| .flatMap { umisByLength => paired.assign(umisByLength) } | |
| .toMap | |
| case _ => | |
| umis | |
| .groupBy(u => u.split("-", -1).map(_.length).toSeq) | |
| .values | |
| .flatMap { _umis => this.assigner.assign(_umis) } | |
| .toMap | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/scala/com/fulcrumgenomics/umi/GroupReadsByUmi.scala` around lines
817 - 835, In the non-paired branch of the rawToId computation (the assigner
match fallback that calls this.assigner.assign/_umis), replace u.split("-") with
u.split("-", -1) so that trailing empty UMI segments are preserved (e.g.,
"ACGT-" remains distinct from "ACGT"); this ensures groupBy(u => u.split("-",
-1).map(_.length).toSeq) produces correct length buckets for
AdjacencyUmiAssigner and prevents mixing raw UMI lengths.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR builds off of a previous effort to allow variable-length UMIs.
Related Issues/PRs