Skip to content

fix(clip): keep MM/ML in step with hard-clipped and unmapped reads - #1012

Merged
nh13 merged 1 commit into
mainfrom
nh/clip-hard-trims-modifications
Oct 2, 2026
Merged

nh13 merged 1 commit into
mainfrom
nh/clip-hard-trims-modifications

Conversation

@nh13

@nh13 nh13 commented Oct 2, 2026 •

Copy link
Copy Markdown
Member

Stacked on #1010 (base nh/em-seq-meth-rev3).

Summary

#1010 made clip --clipping-mode soft-with-mask drop the masked bases' calls from MM/ML. Two cases were still wrong:

  • Hard clipping removed bases from SEQ but left MM/ML (and am/bm) as they were, so the skips pointed past the end of the read or at the wrong cytosines and MN no longer matched SEQ.
  • A read that clipping unmapped: unmapping a reverse-mapped read reverse-complements SEQ, and the edit compared it with the pre-clip SEQ in the other orientation, dropping valid calls.

This adds trim_clipped_modifications_raw, which puts SEQ before and after clipping into MM's original read orientation (each by its own reverse flag), checks that SEQ is now a window of the old SEQ (apart from bases masked to N), drops the calls outside the window or on masked bases, recomputes the skips, and sets MN to the new length when present.

  • clip_template snapshots SEQ, the reverse/unmapped flags and the leading hard clip (all leading H ops) of each record with modification tags. Bases removed from the start show up as added leading hard clip, except when clipping then unmapped the read and cleared its CIGAR: that offset is unknown, so the tags are removed rather than misplaced.
  • Tags that did not fit before clipping or cannot be placed are removed; a record without SEQ (*) is left alone. clip now counts the records that lost their tags and warns at the end of the run.
  • --auto-clip-attributes no longer slices MM/ML/am/bm when their length happens to equal the read's: they are not per-base, and clip trims them itself.

Testing

  • Unit tests for the helper (trimmed start/both ends, a call in the trimmed tail, no MN, an N group, masking, unknown offset, stale MN, a SEQ that is not a window, a reverse read trimmed and unmapped, no SEQ).
  • clip_template tests across the clipper's paths: an existing leading hard clip, upgraded soft clips, auto-clipped attributes, a reverse read clipped completely, a read hard-clipped then unmapped, and a read with a leading hard clip clipped completely.
  • cargo ci-test (11,038 tests), ci-lint, ci-fmt, ci-tag-literals, ci-doc, cargo check -p fgumi-consensus --no-default-features.

Risk verdict: The clip command can now change MM/ML/am/bm and MN output to keep modification calls aligned with clipped SEQ; the new clipping tests pin this behavior. No unsafe change; no CLAUDE.md allowlist update is indicated. No memory-bound, queue-capacity, or thread/backpressure change.

clip trims modification calls that fall outside the retained sequence or on masked bases. It recomputes MM skips and updates an existing MN when possible. If tag placement cannot be established, it removes the affected tags and reports the number of records in a warning. Records without SEQ remain unchanged.

--auto-clip-attributes no longer treats MM/ML/am/bm as per-base attributes based on matching read length. The methylation guide documents the clipping behavior.

Test and CI results are reported in the supplied PR objectives; they were not independently verified here.

@nh13
nh13 deployed to github-actions October 2, 2026 19:13 — with GitHub Actions Active
@coderabbitai

coderabbitai Bot commented Oct 2, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Note

Reviews paused

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: fulcrumgenomics/fgumi/.coderabbit.yaml
  • Review profile: ASSERTIVE
  • Plan: Essentials
  • Run ID: 66b8b88f-3148-4b18-872c-4efc168a3e91
📥 Commits

Reviewing files that changed from the base of the PR and between d773880 and 3770c87.

📒 Files selected for processing (6)
  • crates/fgumi-consensus/src/filter.rs
  • crates/fgumi-consensus/src/modifications.rs
  • crates/fgumi-sam/src/clipper.rs
  • docs/src/guide/methylation.md
  • src/lib/commands/clip.rs
  • src/lib/pipeline/chains/commands/clip.rs

Included review availability: This review used your included allowance. 1 included review remains after this review. Your included PR review attempts over the past 7 days set your current allowance at 2 reviews per hour.


Walkthrough

Clipping now reconciles modification tags with the resulting sequence. The clip command counts records whose tags are removed, and the pipeline warns when that count is nonzero.

Changes

Modification-tag clipping

Layer / File(s) Summary
Reconcile modification tags
crates/fgumi-consensus/src/modifications.rs, crates/fgumi-consensus/src/filter.rs
The new public helper validates the retained sequence, trims calls outside it or on masked bases, updates MN, and removes tags when they cannot be kept consistent. Tests cover clipping offsets, read orientation, stale MN, and invalid sequence windows.
Integrate tag trimming into clipping
crates/fgumi-sam/src/clipper.rs, src/lib/commands/clip.rs, docs/src/guide/methylation.md
The clip command snapshots record state and reconciles modification tags after clipping. Automatic attribute clipping skips modification tags. Tests and guide text cover clipping modes, MN updates, and removed-tag counts.
Aggregate and report tag removals
src/lib/pipeline/chains/commands/clip.rs
Batch processing aggregates records whose modification tags were removed. Finalization warns when the total is greater than zero.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant ClipParams
  participant RawRecordClipper
  participant trim_clipped_modifications_raw
  participant BatchCounts
  ClipParams->>RawRecordClipper: Clip records
  ClipParams->>trim_clipped_modifications_raw: Reconcile tags with clipped sequence
  trim_clipped_modifications_raw-->>ClipParams: Return tag-removal result
  ClipParams->>BatchCounts: Add removal count to clipping outcome
Loading

Merge Risk: ⚪ Minimal · up to 3770c

No identified modification-tag clipping issue remains to address before merging.

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title uses the required Conventional Commit format, includes the affected command scope clip, uses a lowercase imperative description, and accurately describes the main change.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.


Comment @coderabbitai help to get the list of available commands.

@nh13

nh13 commented Oct 2, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai pause

@coderabbitai

coderabbitai Bot commented Oct 2, 2026

Copy link
Copy Markdown
✅ Action performed

Reviews paused.

@codecov

codecov Bot commented Oct 2, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.93252% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 96.42%. Comparing base (d773880) to head (3770c87).

Files with missing lines Patch % Lines
crates/fgumi-consensus/src/modifications.rs 96.66% 2 Missing ⚠️
src/lib/pipeline/chains/commands/clip.rs 94.28% 2 Missing ⚠️
crates/fgumi-sam/src/clipper.rs 88.88% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1012      +/-   ##
==========================================
- Coverage   96.42%   96.42%   -0.01%     
==========================================
  Files         299      299              
  Lines      151026   151142     +116     
==========================================
+ Hits       145623   145733     +110     
- Misses       5403     5409       +6     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@nh13
nh13 force-pushed the nh/em-seq-meth-rev3 branch from 9808fa6 to f170a27 Compare October 2, 2026 19:46
@nh13
nh13 force-pushed the nh/clip-hard-trims-modifications branch from 05d0ab2 to bc5091f Compare October 2, 2026 19:46
@nh13
nh13 deployed to github-actions October 2, 2026 19:47 — with GitHub Actions Active
@nh13
nh13 force-pushed the nh/em-seq-meth-rev3 branch from f170a27 to e671d5e Compare October 2, 2026 19:57
@nh13
nh13 force-pushed the nh/clip-hard-trims-modifications branch from bc5091f to a5a7eeb Compare October 2, 2026 20:00
@nh13
nh13 deployed to github-actions October 2, 2026 20:00 — with GitHub Actions Active
@nh13
nh13 changed the base branch from nh/em-seq-meth-rev3 to main October 2, 2026 20:22
@nh13
nh13 force-pushed the nh/clip-hard-trims-modifications branch from a5a7eeb to f1756cb Compare October 2, 2026 20:28
@nh13
nh13 deployed to github-actions October 2, 2026 20:29 — with GitHub Actions Active
Hard clipping removes bases from SEQ, but clip left MM/ML (and am/bm) as
they were. The skips then pointed past the end of the read or at the
wrong cytosines, and MN no longer matched SEQ, so MM/ML consumers skipped
the record and a later filter removed the tags. A read that clipping
unmapped was also mishandled: unmapping a reverse-mapped read
reverse-complements SEQ, and the soft-with-mask edit compared it with the
pre-clip SEQ in the other orientation, dropping valid calls.

Add trim_clipped_modifications_raw. It puts SEQ before and after
clipping into MM's original read orientation, each by its own reverse
flag, checks that SEQ is now a window of the old SEQ (apart from bases
masked to N), drops the calls outside the window or on masked bases,
recomputes the skips over the window, and sets MN to the new length when
present. An unmapped reverse read, whose SEQ only changed orientation,
keeps its tags. clip_template snapshots SEQ, the reverse and unmapped
flags and the leading hard clip (all leading H ops) of each record with
modification tags; bases removed from the start show up as added
leading hard clip, except when clipping then unmapped the read and
cleared its CIGAR, where the offset is unknown and the tags are removed
rather than misplaced. Tags that did not fit before clipping or cannot
be placed are removed, a record without SEQ is left alone, and clip now
counts the records that lost their tags and warns at the end of the run.

--auto-clip-attributes no longer slices MM/ML/am/bm when their length
happens to equal the read's: they are not per-base, and clip trims them
itself. The methylation guide and the option's help say so.
@nh13
nh13 force-pushed the nh/clip-hard-trims-modifications branch from f1756cb to 3770c87 Compare October 2, 2026 21:29
@nh13
nh13 deployed to github-actions October 2, 2026 21:29 — with GitHub Actions Active
@nh13

nh13 commented Oct 2, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Oct 2, 2026 •

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@nh13
nh13 added this pull request to the merge queue Oct 2, 2026
Merged via the queue into main with commit 33126e6 Oct 2, 2026
21 checks passed
@nh13
nh13 deleted the nh/clip-hard-trims-modifications branch October 2, 2026 22:29
@nh13 nh13 mentioned this pull request Oct 2, 2026

This branch was successfully deployed

1 active deployment
github-actions — 3770c870 Deployed Oct 2, 2026 by nh13 via coverage #4784
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant