Skip to content

Conversation

@ljwharbers
Copy link
Contributor

@ljwharbers ljwharbers commented Aug 13, 2025

This adds the ability to include an index of choice using samtools sort. Currently it was only possible to use the ext.args = "--write-index" which will only output a .csi index. This adds a new val channel input that will output the selected bam/sam/cram index. If the channel is left empty '' no index will be written and the old behaviour is maintained.

This is the same as the use case in the latest version of samtools/view and minimap2/align and will reduce a lot of unecessary SAMTOOLS_SORT into SAMTOOLS_INDEX processes.

Due to many modules/subworkflows use samtools sort (for instance in BAM_SORT_STATS_SAMTOOLS), other tests are also updated to include an empty third input channel to maintain the old behaviour.

Optionally BAM_SORT_STATS_SAMTOOLS can also be updated to now entirely skip SAMTOOLS_INDEX and simply directly write the bai index within SAMTOOLS_SORT

PR checklist

Closes #XXX

  • 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.
  • Emit the versions.yml file.
  • 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 <MODULE> --profile docker
      • nf-core modules test <MODULE> --profile singularity
      • nf-core modules test <MODULE> --profile conda
    • For subworkflows:
      • nf-core subworkflows test <SUBWORKFLOW> --profile docker
      • nf-core subworkflows test <SUBWORKFLOW> --profile singularity
      • nf-core subworkflows test <SUBWORKFLOW> --profile conda

@ljwharbers
Copy link
Contributor Author

@nf-core-bot update gpu snapshot path: subworkflows/nf-core/fastq_align_dedup_bwameth

@ljwharbers ljwharbers marked this pull request as ready for review August 13, 2025 15:29
Copy link
Contributor

@fellen31 fellen31 left a comment

Choose a reason for hiding this comment

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

Looks good, just one thing.

args.contains("--output-fmt cram") ? "cram" :
"bam"
reference = fasta ? "--reference ${fasta}" : ""
output_file = index_format ? "${prefix}.${extension}##idx##${prefix}.${extension}.${index_format} --write-index" : "${prefix}.${extension}"
Copy link
Contributor

Choose a reason for hiding this comment

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

I would add a check for this here and in the stub.

if (index_format) {
        if (!index_format.matches('bai|csi|crai')) {
            error "Index format not one of bai, csi, crai."
        } else if (extension == "sam") {
            error "Indexing not compatible with SAM output"
        }
    }

Since technically, index_format can be anything, but we only have outputs for bai, crai, csi (I'm team emit: index, but that's another discussion).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, I would also prefer a simple emit: index option. Either way, I've added the check like this.

@ljwharbers
Copy link
Contributor Author

@fellen31 Thanks for reviewing! So it's all good to merge once checks are complete or does it need approval of anyone else first still?

@fellen31
Copy link
Contributor

I say good to merge!

Perhaps some extra care could be taken since changing samtools sort might affect many pipelines, but we had the discussion already for samtools view and came to a conclusion (#4326), and this implementation is exactly the same.

@ljwharbers ljwharbers added this pull request to the merge queue Aug 18, 2025
Merged via the queue into nf-core:master with commit e89ff68 Aug 18, 2025
70 checks passed
@ljwharbers ljwharbers deleted the samtools_sort branch August 18, 2025 12:01
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.

3 participants