Skip to content

feat(alignmentSieve): opt-in --keepTags for shifted BAM output - #1494

Open
adRn-s wants to merge 6 commits into
masterfrom
alignmentsieve-keep-tags
Open

adRn-s wants to merge 6 commits into
masterfrom
alignmentsieve-keep-tags

Conversation

@adRn-s

@adRn-s adRn-s commented Oct 24, 2026 •

Copy link
Copy Markdown
Collaborator
  • Does the PR contain new feature?
  • Does the PR contain bugfix?
  • Does the PR contain documentation changes?
  • Does the PR contain changes to the galaxy wrapper?
  • Does the PR title follow Conventional Commits?

This is the alignmentSieve half of the reverted #1485, split out and reworked after received input comments. It does not depend on the crate PR.

What changes

New option --keepTags TAG [TAG ...]. With --shift or --ATACshift and BAM output it copies the named tags (for example CB) onto the shifted records. Without it nothing changes: SEQ, QUAL and all tags are dropped like on master. I did not touch the shift code itself.

Tags that describe the original alignment are refused with an error: NM, MD, MC, SA, XA, CC, CP, CG, OA, OC, OP, TS. The CIGAR is rewritten, so they would be wrong. --keepTags also errors without --shift/--ATACshift or together with --BED. The docs page has a new section on what shifted BAM output contains.

The use case is single-cell data, where the shifted reads still need their cell barcode.

Potential Concerns

  • Performance. The default path is untouched. On a 382 MB BAM (7.9M records, 8 threads) --ATACshift --keepTags CB took about 14 s against about 13 s for the default, so barely slower. Only CB is written, so the output stays small. These are a handful of runs on one machine, so treat the number as approximate.
  • Tags. Only the "core" tags you are kept, and the alignment tags above are rejected instead of silently carried along.
  • Expanding shifts and shifts longer than the read. My first version of the branch broke both (every read was dropped for --shift -10 10 and --shift 100 -100). I reset the branch to master and redid it as an opt-in flag. I re-checked those shifts on paired_chr2L.bam and they behave as on master again.
  • Edge cases. Documented in alignmentSieve.rst.

How it was checked

  • cargo test --features python and the alignmentSieve plus bamCoverage/bamCompare pytest files.
  • New tests: tags dropped by default, CB kept with unchanged positions and values, alignment tags rejected, and the shift and BAM output requirement.
  • Run on the large BAM above, checking that every record carries only CB.

What requires further feedback

  1. Is an opt-in flag the right shape, or would we rather change the default to keep a small set of tags?
  2. I did not look at the mate fields (mpos, tlen) when only one read of a pair moves. That is master's behaviour and I left it alone, but we should know that code could be revised later on.
  3. I haven't checked how the Galaxy wrapper would use --keepTags. Happy to add it here, or later.

Default shifted output is unchanged (SEQ, QUAL and all tags dropped).
--keepTags TAG [TAG ...] copies only the named tags (e.g. CB). Tags that
describe the original alignment (NM, MD, MC, SA, XA, ...) are rejected
because the CIGAR is rewritten. Needs --shift/--ATACshift and BAM output.
Shift handling is untouched, so expanding and long shifts keep working.
@coderabbitai

coderabbitai Bot commented Oct 24, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

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
Walkthrough

The change adds --keepTags for shifted BAM output. The CLI validates tag selections, and Rust copies available selected tags from original reads to shifted records. Tests and documentation describe the option and its behavior.

Changes: Shifted BAM tag retention

Layer / File(s) Summary
Define and validate tag selection
pydeeptools/deeptools/alignmentSieve2.py
The CLI adds --keepTags, rejects malformed or refused tags, requires shifting and BAM output, and passes selections to Rust.
Preserve selected tags on shifted records
src/alignmentsieve.rs, pydeeptools/deeptools/test/test_alignmentsieve.py, docs/content/tools/alignmentSieve.rst
Rust copies requested tags from original reads when available. Tests check default tag removal, retained CB values, and invalid uses. Documentation describes shifted BAM output and option constraints.

Priority: Normal

Change: Feature

Merge Risk: Low · up to 2e932

Most users are unaffected, but repeated CLI options can silently omit requested tags and Galaxy users cannot access the new feature through the wrapper.

Security architecture risk: Low · up to 2e932

The option is explicit and preserves default behavior. No new privilege or cross-tenant access was identified. The remaining design uncertainty concerns metadata-safety checks and compatibility for applications that call the transformation directly.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The demonstrated added exposure is selected input metadata appearing in the caller-selected shifted BAM output. The new selection does not choose another destination or acquire additional authority.

Trust Boundaries and Controls

  • observed — The normal CLI path enforces metadata-safety policy before native execution. Direct calls to the exported transformation do not receive equivalent checks. This establishes an enforcement difference, but not an authentication or tenant-isolation bypass.

Resilience and Maintainability Implications

  • inferred — Per-record cloning and invocation-local selection avoid introducing shared retained-tag state across records or runs. Repetition does not introduce a reservation, ownership-transfer, or recovery protocol; destination-file coordination remains unchanged.
Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 53.85% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 13 functions across 3 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Title check Passed The title clearly identifies the new opt-in --keepTags feature for shifted alignmentSieve BAM output.
Description check Passed The description explains the new option, constraints, rejected tags, documentation, tests, and known concerns. It directly matches the changeset.
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.
Finishing Touches: 💡 1
📝 Generate docstrings: 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

coderabbitai[bot]

This comment was marked as resolved.

coderabbitai[bot]

This comment was marked as resolved.

…n, reject AS

A multi-byte tag such as 'éA' passed the length check and panicked in Rust.
Require [A-Za-z][A-Za-z0-9] in the CLI. AS describes the original alignment
score, so it joins the rejected tags.
coderabbitai[bot]

This comment was marked as resolved.

Shifted records have empty SEQ and QUAL, so tags that depend on the original
bases (BQ, MM, ML) would be stale.
@adRn-s
adRn-s requested a review from WardDeb October 26, 2026 10:02
coderabbitai[bot]

This comment was marked as resolved.

CM and PT depend on the original alignment and go stale after the CIGAR rewrite. A repeated tag would be written twice on one record.
coderabbitai[bot]

This comment was marked as resolved.

… Galaxy wrapper

argparse store kept only the last --keepTags group, so the duplicate check missed repeats and earlier tags were dropped.
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