feat(group): accept query-grouped unaligned input with --allow-unmapped - #620
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)
Walkthrough
ChangesGroup input sort validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant GroupReadsByUmi
participant InputHeader
participant Diagnostics
GroupReadsByUmi->>InputHeader: classify sort order and grouping tags
alt template-coordinate sorted
InputHeader-->>GroupReadsByUmi: accept input
else query-grouped with --allow-unmapped
InputHeader-->>GroupReadsByUmi: accept query-grouped input
GroupReadsByUmi->>Diagnostics: log acceptance and warn when `@SQ` is present
else unusable or unauthorized ordering
GroupReadsByUmi->>Diagnostics: report order-specific remediation
end
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #620 +/- ##
==========================================
+ Coverage 93.42% 93.51% +0.08%
==========================================
Files 175 175
Lines 104807 105772 +965
==========================================
+ Hits 97919 98913 +994
+ Misses 6888 6859 -29 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
7a714d2 to
1b1ac60
Compare
|
@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/group.rs`:
- Around line 982-1012: Update the query_grouped_unmapped classification in the
command’s header-validation flow so it is true only when --allow-unmapped is
enabled and the input is query-grouped but not template-coordinate sorted.
Preserve the existing rejection checks, and ensure genuinely
template-coordinate-sorted headers—including those with `@SQ` records—follow the
“Input is template-coordinate sorted” branch and do not emit the query-grouped
warning.
🪄 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: b3e35d5a-ddee-4f5f-bb9a-ab0862642740
📒 Files selected for processing (2)
src/lib/commands/group.rssrc/lib/sam/mod.rs
`group` required template-coordinate sort unconditionally, so a user with an unaligned BAM who passed `--allow-unmapped` -- whose entire purpose is to place all unmapped reads in a single position group -- was still told to run `fgumi sort --order template-coordinate` on data that has no coordinates. That blocked the unaligned-only workflow (ribosome display, for example) with advice that could not be followed. Template-coordinate sort exists to make position adjacency match the grouping key. Under `--allow-unmapped` every unmapped record lands in one group whatever the order, so that guarantee buys nothing. Query grouping is still required, for a different reason: templates are assembled by comparing each record's name to the previous one, so without adjacency an R1/R2 pair splits into two partial templates. That is corruption rather than a policy choice, so it stays enforced -- and the error for that case points at `--order queryname` rather than the template-coordinate remediation, which would not help a file with no coordinates. Whether the header declares reference sequences is deliberately not part of the decision. An all-unmapped BAM keeps its `@SQ` lines after something like `samtools view -f 4`, and gating on their absence would reject it for no benefit. Mapped reads in query-grouped input are simply not position-adjacent, so each mapped template forms its own family -- that under-groups rather than merging unrelated molecules, and a warning names the case when the header declares references. Without `--allow-unmapped` nothing changes: template-coordinate sort is still required, including for extract-style `SO:unsorted GO:query` headers with no `SS`, which the existing regression test continues to pin. The accept/reject decision is a three-way classification, not two overlapping booleans, so it lives in `classify_input_ordering`. Deciding it as booleans hid a bug: template-coordinate headers advertise `GO:query`, so they also satisfy `is_query_grouped`, and under `--allow-unmapped` a correctly-sorted BAM was reported as merely query-grouped -- warning that its mapped templates "will not be position-grouped" when they are, and making the template-coordinate log line unreachable. Classifying template-coordinate first fixes it, and a case table pins which ordering each header resolves to rather than only accept-vs-reject. Verified end-to-end: `extract` output groups correctly through the new path, an all-unmapped BAM that declares `@SQ` does too, and input without the flag is still rejected.
1b1ac60 to
d570e94
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
This branch was previously deployed
1 participant
grouprequired template-coordinate sort unconditionally. So a user with an unaligned BAM who passed--allow-unmapped— whose entire purpose, per the command's own log line, is "All unmapped reads will form a single position group per library/cell" — was still told:on data that has no coordinates. That blocked the unaligned-only workflow (ribosome display, for instance) with advice the user cannot act on.
What changed
Template-coordinate sort exists to make position adjacency match the grouping key. Under
--allow-unmappedevery unmapped record lands in one group whatever the order, so that guarantee buys nothing — and the requirement is dropped.Query grouping is still required, for a different reason. Templates are assembled by comparing each record's name to the previous one (
group_by_name_and_build), so without adjacency an R1/R2 pair splits into two partial templates. That is corruption rather than a policy choice, so it stays enforced — and its error points at--order queryname, since the template-coordinate remediation is useless for a file with no coordinates.Whether the header declares
@SQis deliberately not part of the decision. An earlier revision gated on their absence; that would reject an all-unmapped BAM that kept its header through something likesamtools view -f 4, for no benefit. Mapped reads in query-grouped input are simply not position-adjacent, so each mapped template forms its own family — that under-groups rather than merging unrelated molecules, and--allow-unmappedalready warns loudly that it groups by UMI alone. A further warning names the case explicitly when the header declares references.Note there is no fgbio precedent to follow here: fgbio has no
--allow-unmappedat all, so this is fgumi-specific semantics.What did not change
Without
--allow-unmapped, template-coordinate sort is still required — including for extract-styleSO:unsorted GO:queryheaders with noSS. That case is the regression test for a real earlier bug where such headers were silently mistaken for template-coordinate, and it keeps failing without the flag. Accepting it under the flag routes it to the unmapped-only grouping path deliberately, which is a different thing from mistaking it for a sorted file.The pre-existing rejection test previously asserted rejection under both flag settings; it is now scoped to the no-flag contract, with the flag contract covered by the new test. Both are explicit rather than one being weakened.
Verification
End-to-end with the real binary, not just unit tests:
fgumi extractoutput (SO:unsorted GO:query, no@SQ) → 12/12 records out, 2 MI groups matching the 2 distinct UMIs@SQ→ same result, plus the warning--allow-unmapped→ still rejectedcargo ci-test(5536 tests),cargo ci-fmt,cargo ci-lint,RUSTDOCFLAGS="-D warnings" cargo ci-docpass.Summary by CodeRabbit
--allow-unmappedcan now handle query-grouped inputs even when reads are not adjacent by template-coordinate sorting.--ordersetting for each input type.--allow-unmappedis not enabled.