Skip to content

Conversation

@mkatsanto
Copy link
Contributor

@mkatsanto mkatsanto commented Oct 29, 2025

PR checklist

Fixes #9265

Addition of the vembrane sort module https://github.com/vembrane/vembrane

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

@mkatsanto mkatsanto requested a review from famosab October 29, 2025 10:44
@mkatsanto mkatsanto moved this from To do to Ready for review in Hackathon Barcelona October 2025 Oct 29, 2025
@mkatsanto mkatsanto self-assigned this Oct 29, 2025
@mkatsanto mkatsanto changed the title Vembrane sort feat: addition of vembrane sort module Oct 29, 2025
then {
assertAll(
{ assert process.success },
{ assert snapshot(process.out).match() }
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
{ assert snapshot(process.out).match() }
{ assert snapshot(
process.out,
path(process.out.versions[0]).yaml
).match() }

Copy link
Contributor

Choose a reason for hiding this comment

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

This is added in the other PR right?, can you remove it here? :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, what I meant: can you please do one PR per module :) meaning one seperate one for vembrane/table?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you run

nextflow lint -format -sort-declarations -spaces 4 -harshil-alignment modules/nf-core/vembrane/sort/main.nf

please? :)

@famosab
Copy link
Contributor

famosab commented Oct 29, 2025

vembrane sort: error: argument --output-fmt/-O: invalid choice: 'vcf.gz' (choose from vcf, bcf, uncompressed-bcf)

@famosab
Copy link
Contributor

famosab commented Oct 29, 2025

Seems like gzipped vcfs are not allowed. I would suggest to open an issue over at the vembrane repo to request that and remove it from the module for now :) You can link to the PR in the issue you open :)

Comment on lines 89 to 114
test("homo_sapiens - [vcf] - vcf - compressed_output") {

config "./nextflow.config"

when {
params {
vembrane_args = '--output-fmt vcf.gz'
}
process {
"""
input[0] = [
[ id:'test_gz' ], // meta map
file(params.modules_testdata_base_path + 'genomics/homo_sapiens/illumina/vcf/test.rnaseq.vcf', checkIfExists: true)
]
input[1] = 'QUAL'
"""
}
}

then {
assertAll(
{ assert process.success },
{ assert snapshot(process.out).match() }
)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
test("homo_sapiens - [vcf] - vcf - compressed_output") {
config "./nextflow.config"
when {
params {
vembrane_args = '--output-fmt vcf.gz'
}
process {
"""
input[0] = [
[ id:'test_gz' ], // meta map
file(params.modules_testdata_base_path + 'genomics/homo_sapiens/illumina/vcf/test.rnaseq.vcf', checkIfExists: true)
]
input[1] = 'QUAL'
"""
}
}
then {
assertAll(
{ assert process.success },
{ assert snapshot(process.out).match() }
)
}
}

- "*.vcf*":
type: file
description: Sorted VCF file (can be compressed)
pattern: "*.{vcf,vcf.gz}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
pattern: "*.{vcf,vcf.gz}"
pattern: "*.{vcf,bcf}"

val expression

output:
tuple val(meta), path("*.vcf*"), emit: vcf
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
tuple val(meta), path("*.vcf*"), emit: vcf
tuple val(meta), path("*.{vcf.gz,vcf,bcf}"), emit: vcf

Copy link
Contributor

Choose a reason for hiding this comment

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

or rather without vcf.gz

Comment on lines 24 to 25
def suffix = args.contains('--output-fmt vcf.gz')
? 'vcf.gz'
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought this is failing 🤔

@famosab
Copy link
Contributor

famosab commented Nov 6, 2025

@mkatsanto do you have time to finish this? :) then I would give it another round of review - or do you need some help / me to pick it up (both would be fine!)

@mkatsanto
Copy link
Contributor Author

mkatsanto commented Nov 6, 2025

@mkatsanto do you have time to finish this? :) then I would give it another round of review - or do you need some help / me to pick it up (both would be fine!)

@famosab I can do this tomorrow, but if you are in a hurry please go ahead!

@famosab
Copy link
Contributor

famosab commented Nov 6, 2025

No, no hurry at all :) I was just wondering because we never talked about it during the Hackathon! Thank you!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Ready for review

Development

Successfully merging this pull request may close these issues.

new module: vembrane

3 participants