Skip to content

Samtools mpileup input update#9810

Merged
Krannich479 merged 14 commits intonf-core:masterfrom
Krannich479:samtools_mpileup_input_update
Feb 2, 2026
Merged

Samtools mpileup input update#9810
Krannich479 merged 14 commits intonf-core:masterfrom
Krannich479:samtools_mpileup_input_update

Conversation

@Krannich479
Copy link
Contributor

@Krannich479 Krannich479 commented Jan 30, 2026

PR checklist

Closes #9802

  • 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 <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

An extensive description of why I believe a new input section is necessary can be found in #9802 .

@Krannich479 Krannich479 self-assigned this Jan 30, 2026
@Krannich479 Krannich479 added the enhancement New feature or request label Jan 30, 2026
@Krannich479
Copy link
Contributor Author

With commit 59c4331 :

  • the input rolled back to the previous original format, just the index added to the tuple
  • samtools version rolled back to 1.22.1 s.t. it matches the other modules again
  • minor documentation fixes

@@ -49,7 +49,7 @@ input:
description: FASTA reference file. Optional.
Copy link
Contributor

Choose a reason for hiding this comment

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

fasta will also be required when using cram inputs, but that's implied and known when using cram data I guess

Copy link
Contributor

@matthdsm matthdsm left a comment

Choose a reason for hiding this comment

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

LGTM!
Thanks for wading through all the input

@Krannich479
Copy link
Contributor Author

Thank you for all your valuable feedback!

@Krannich479 Krannich479 added this pull request to the merge queue Feb 2, 2026
Merged via the queue into nf-core:master with commit 4a49a1a Feb 2, 2026
39 checks passed
@Krannich479 Krannich479 deleted the samtools_mpileup_input_update branch February 2, 2026 15:14
@Krannich479
Copy link
Contributor Author

I leave this here for documentation:
The way I parallelized SAMTOOLS_MPILEUP across chromosomes now is by passing the chromosome identifier to -r via the meta map.

    withName: 'SAMTOOLS_MPILEUP' {
        ext.args    = { "-r ${meta.region}" }
        ext.prefix  = { "${meta.id}.${meta.region}" }
    }

cavenel pushed a commit to cavenel/modules that referenced this pull request Feb 5, 2026
* new input layout; samtools version bump

* tests for new input layout

* fix linting issues

* prettier shenanigans in CI

* Fix formatting and indentation in main.nf.test

* change input layout to one large tuple for all sample related input

* added faidx optional input parameter

* tiny name fix

* tiny name fix in meta too
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request Ready for Review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Samtools mpileup requires another input parameter to parallelize across chromosomes/regions

4 participants

Comments