Skip to content

samtools/merge: emit .bai/.crai index files by default (fixes #4261) - #12859

Merged
SPPearce merged 12 commits into
nf-core:masterfrom
ramakrishna-p21:update-samtools-merge-index
Sep 10, 2026
Merged

samtools/merge: emit .bai/.crai index files by default (fixes #4261)#12859
SPPearce merged 12 commits into
nf-core:masterfrom
ramakrishna-p21:update-samtools-merge-index

Conversation

@ramakrishna-p21

Copy link
Copy Markdown
Contributor

PR checklist

Closes #4261

  • This comment contains a description of changes (with reason).
  • If you've fixed a bug or added code that should be tested, add tests!
  • If you've added a new tool - have you followed the module conventions in the contribution docs
  • If necessary, include test data in your PR.
  • Remove all TODO statements.
  • Broadcast software version numbers to topic: versions - See version_topics
  • Follow the naming conventions.
  • Follow the parameters requirements.
  • Follow the input/output options guidelines.
  • Add a resource label
  • Use BioConda and BioContainers if possible to fulfil software requirements.
  • Ensure that the test works with either Docker / Singularity. Conda CI tests can be quite flaky:
    • For modules:
      • nf-core modules test samtools/merge --profile docker

Description of Changes

This PR updates samtools/merge to automatically generate and emit index files (.bai for BAM and .crai for CRAM) alongside the merged alignment file, resolving #4261.

Key Modifications

  • main.nf: Added --write-index and ##idx## syntax to the samtools merge command execution to emit .bai for BAM files and .crai for CRAM files by default. Updated the stub: block to touch the corresponding index file.
  • meta.yml: Updated documentation for the emitted index channel pattern.
  • tests/main.nf.test: Removed manual --write-index overrides from module test configs.
  • tests/main.nf.test.snap: Refreshed snapshots to reflect the generated .bai and .crai indices.

Note: Developed and verified with assistance from Antigravity. All test fixtures, linting, and biological logic were manually reviewed and validated against nf-test.

@SPPearce SPPearce left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if the bam/cram files are not in coordinate sort order?

@SPPearce

SPPearce commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Now I could be convinced to have an input channel to make the indexes, rather than the horrific --write-index mess.

@ramakrishna-p21

Copy link
Copy Markdown
Contributor Author

Thanks for the review, @SPPearce. That's a great point about the sorting edge cases, and I agree on dropping the ##idx## syntax.

If we handle this through the input channel, would adding an optional boolean (like val(create_index)) to trigger a separate samtools index step work best for you?

Let me know how you'd prefer the input signature structured so we don't break existing pipelines, and I'll get the PR updated!

@SPPearce

SPPearce commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Hi @ramakrishna-p21 , can you take a look at how samtools/sort implements this, with the input channel there.
I've had approval from the other maintainers to have this input channel like this.

@SPPearce

SPPearce commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Also please join the community via the slack channel github-invitations.

Replaces hardcoded --write-index with an explicit index_format input, mirroring the samtools/sort architecture. Updates module tests and bam_applybqsr subworkflow.
@ramakrishna-p21

Copy link
Copy Markdown
Contributor Author

Hey @SPPearce, I've refactored the module to use the index_format input channel, matching the samtools/sort architecture exactly. I also updated the downstream bam_applybqsr subworkflow to dynamically pass crai or bai based on the output format, and regenerated all tests and snapshots. Please take a look at the updated changes when you have a moment. Also, I've just submitted my GitHub invitation request in the slack channel as suggested!

Comment thread subworkflows/nf-core/bam_applybqsr/main.nf Outdated
Comment thread modules/nf-core/samtools/merge/meta.yml Outdated
Comment thread modules/nf-core/samtools/merge/tests/main.nf.test Outdated
ramakrishna-p21 and others added 4 commits September 9, 2026 10:33
Co-authored-by: Simon Pearce <24893913+SPPearce@users.noreply.github.com>
Co-authored-by: Simon Pearce <24893913+SPPearce@users.noreply.github.com>
… bam_applybqsr snapshots

Generated by Antigravity
@ramakrishna-p21

Copy link
Copy Markdown
Contributor Author

Hi @SPPearce, I've updated everything based on your feedback:

  • Swapped bai to csi in bam_applybqsr and re-recorded the snapshot

  • Added the sorting note to meta.yml

  • Cleaned up main.nf.test with sanitizeOutput(readsMD5Keys: ...) and refreshed the snapshots

Local tests and linting are all green. Let me know if you spot anything else!

@SPPearce SPPearce left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!
Minor tweak to use the test data param.

Comment thread modules/nf-core/samtools/merge/tests/main.nf.test Outdated
Comment thread modules/nf-core/samtools/merge/tests/main.nf.test Outdated
ramakrishna-p21 and others added 2 commits September 9, 2026 13:08
Co-authored-by: Simon Pearce <24893913+SPPearce@users.noreply.github.com>
Co-authored-by: Simon Pearce <24893913+SPPearce@users.noreply.github.com>
@SPPearce

SPPearce commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Huh, looks like that didn't work. So just put it back to how it was

@ramakrishna-p21

Copy link
Copy Markdown
Contributor Author

Done, put it back to how it was! Tests and linting are all green again. Feel free to merge whenever you're ready!

@SPPearce
SPPearce added this pull request to the merge queue Sep 10, 2026
Merged via the queue into nf-core:master with commit 8fd5fe1 Sep 10, 2026
62 checks passed
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.

Add .bai as an optional output of samtools/merge

3 participants