Add virusrecom module for viral recombination detection#11658
Open
haggaikelisha-ai wants to merge 12 commits into
Open
Add virusrecom module for viral recombination detection#11658haggaikelisha-ai wants to merge 12 commits into
haggaikelisha-ai wants to merge 12 commits into
Conversation
Author
|
@nf-core-bot fix linting |
Joon-Klaps
requested changes
May 18, 2026
Contributor
Joon-Klaps
left a comment
Joon-Klaps
left a comment
There was a problem hiding this comment.
Very Nice contribution.
I believe this module is going to be a bit more difficult (still totally feasible) to add, especially for a first module.
Let me know what you think, open to discussion.
If you do decide to continue, make sure all possible input combinations are tested & that we update the meta.yml file
| tag "modules_nfcore" | ||
| tag "virusrecom" | ||
|
|
||
| test("virusrecom - fasta - stub") { |
Contributor
There was a problem hiding this comment.
Don't forget to make an actuall test once the testdata is merged in.
Would also be better if the stub used these files as well instead of fasta & fasta.fai
Suggested change
| test("virusrecom - fasta - stub") { | |
| test("sarscov2 - alignment [fasta] mapping [tsv] - stub") { |
| then { | ||
| assert process.success | ||
| assertAll( | ||
| { assert snapshot(process.out).match() } |
Contributor
There was a problem hiding this comment.
Suggested change
| { assert snapshot(process.out).match() } | |
| { assert snapshot(sanitizeOutput(process.out)).match() } |
sanitizeOutput() makes the snapshot easier to read.
| @@ -0,0 +1,38 @@ | |||
| process VIRUSRECOM { | |||
| tag "$meta.id" | |||
| label 'process_single' | |||
Contributor
There was a problem hiding this comment.
Suggested change
| label 'process_single' | |
| label 'process_low' |
We can specify the number of threads to use but we would restrict it to only a single thread? From their docs the default is 4. I'm not familar with the tool, but if it takes a long time, we might want to update to even process_medium
|
|
||
| output: | ||
| tuple val(meta), path("${meta.id}/"), emit: results | ||
| tuple val("${task.process}"), val('virusrecom'), eval("virusrecom -h 2>&1 | grep 'Version:' | sed 's/.*Version: //' | sed 's/ (.*//'" ), topic: versions, emit: versions_virusrecom |
Contributor
There was a problem hiding this comment.
Suggested change
| tuple val("${task.process}"), val('virusrecom'), eval("virusrecom -h 2>&1 | grep 'Version:' | sed 's/.*Version: //' | sed 's/ (.*//'" ), topic: versions, emit: versions_virusrecom | |
| tuple val("${task.process}"), val('virusrecom'), eval("virusrecom -h 2>&1 | sed -n 's/.*Version: \([^ ]*\).*/\1/p'"), topic: versions, emit: versions_virusrecom |
It's shorter
| def prefix = task.ext.prefix ?: "${meta.id}" | ||
| """ | ||
| virusrecom \\ | ||
| -a ${alignment} \\ |
Contributor
There was a problem hiding this comment.
Looking at the docs, we can also pass an unaligned file and have it aligned with mafft or another tool as well as passa a iwic input.
If we were to follow the nf-core rules, we should also support this.
I believe the best approach to be is,
input:
tuple val(meta), path(fasta), path(mapping), path(iwic)
val(is_aligned)
val(alignment_tool)
script:
def args = task.ext.args ?: ''
def prefix = task.ext.prefix ?: "${meta.id}"
def use_iwic = iwic as boolean // path(iwic) will be [] when absent
def input_command = use_iwic ? "-iwic ${iwic}"
: is_aligned ? "-a ${fasta}"
: "-ua ${fasta}"
def map_command = use_iwic ? "" : "-map ${mapping}"
def at_command = (!use_iwic && !is_aligned && alignment_tool) ? "-at ${alignment_tool}" : ""
if (!use_iwic && !is_aligned && !alignment_tool) {
error "alignment_tool must be specified when input is unaligned (no iwic provided)"
}
"""
virusrecom \\
${input_command} \\
${map_command} \\
${at_command} \\
-o ${prefix} \\
-t ${task.cpus} \\
${args}
"""
Author
|
Hello i am working on it but i may need guide. Let me send what i have and
evaluate so far on it.
Thank you
…On Mon, 18 May 2026 at 11:28, Joon Klaps ***@***.***> wrote:
***@***.**** requested changes on this pull request.
Very Nice contribution.
I believe this module is going to be a bit more difficult (still totally
feasible) to add, especially for a first module.
Let me know what you think, open to discussion.
If you do decide to continue, make sure all possible input combinations
are tested & that we update the meta.yml file
------------------------------
In modules/nf-core/virusrecom/tests/main.nf.test
<#11658 (comment)>:
> @@ -0,0 +1,29 @@
+nextflow_process {
+ name "Test Process VIRUSRECOM"
+ script "../main.nf"
+ process "VIRUSRECOM"
+ tag "modules"
+ tag "modules_nfcore"
+ tag "virusrecom"
+
+ test("virusrecom - fasta - stub") {
Don't forget to make an actuall test once the testdata is merged in.
Would also be better if the stub used these files as well instead of fasta
& fasta.fai
⬇️ Suggested change
- test("virusrecom - fasta - stub") {
+ test("sarscov2 - alignment [fasta] mapping [tsv] - stub") {
------------------------------
In modules/nf-core/virusrecom/tests/main.nf.test
<#11658 (comment)>:
> + options "-stub"
+ when {
+ process {
+ """
+ input[0] = [
+ [ id:'test', single_end:false ],
+ file(params.modules_testdata_base_path + 'genomics/sarscov2/genome/genome.fasta', checkIfExists: true),
+ file(params.modules_testdata_base_path + 'genomics/sarscov2/genome/genome.fasta.fai', checkIfExists: true)
+ ]
+ """
+ }
+ }
+ then {
+ assert process.success
+ assertAll(
+ { assert snapshot(process.out).match() }
⬇️ Suggested change
- { assert snapshot(process.out).match() }
+ { assert snapshot(sanitizeOutput(process.out)).match() }
sanitizeOutput()
<https://nf-co.re/nft-utils#sanitizeoutput---sanitize-process-output-to-create-clean-snapshots>
makes the snapshot easier to read.
------------------------------
In modules/nf-core/virusrecom/main.nf
<#11658 (comment)>:
> @@ -0,0 +1,38 @@
+process VIRUSRECOM {
+ tag "$meta.id"
+ label 'process_single'
+
+ conda "${moduleDir}/environment.yml"
+ container "${ workflow.containerEngine in ['singularity', 'apptainer'] && !task.ext.singularity_pull_docker_container ?
+ 'https://depot.galaxyproject.org/singularity/virusrecom:1.4.0--pyhdfd78af_0':
+ 'quay.io/biocontainers/virusrecom:1.4.0--pyhdfd78af_0' }"
⬇️ Suggested change
- 'quay.io/biocontainers/virusrecom:1.4.0--pyhdfd78af_0' }"
+ 'biocontainers/virusrecom:1.4.0--pyhdfd78af_0' }"
By default we use quay.io, users can specify their own registery if they
want. See the docs
<https://nf-co.re/docs/running/configuration/nextflow-for-your-system#customize-docker-registries>
------------------------------
In modules/nf-core/virusrecom/main.nf
<#11658 (comment)>:
> @@ -0,0 +1,38 @@
+process VIRUSRECOM {
+ tag "$meta.id"
+ label 'process_single'
⬇️ Suggested change
- label 'process_single'
+ label 'process_low'
We can specify the number of threads to use but we would restrict it to
only a single thread? From their docs the default is 4. I'm not familar
with the tool, but if it takes a long time, we might want to update to even
process_medium
------------------------------
In modules/nf-core/virusrecom/main.nf
<#11658 (comment)>:
> @@ -0,0 +1,38 @@
+process VIRUSRECOM {
+ tag "$meta.id"
+ label 'process_single'
+
+ conda "${moduleDir}/environment.yml"
+ container "${ workflow.containerEngine in ['singularity', 'apptainer'] && !task.ext.singularity_pull_docker_container ?
+ 'https://depot.galaxyproject.org/singularity/virusrecom:1.4.0--pyhdfd78af_0':
+ 'quay.io/biocontainers/virusrecom:1.4.0--pyhdfd78af_0' }"
+
+ input:
+ tuple val(meta), path(alignment), path(mapping)
+
+ output:
+ tuple val(meta), path("${meta.id}/"), emit: results
+ tuple val("${task.process}"), val('virusrecom'), eval("virusrecom -h 2>&1 | grep 'Version:' | sed 's/.*Version: //' | sed 's/ (.*//'" ), topic: versions, emit: versions_virusrecom
⬇️ Suggested change
- tuple val("${task.process}"), val('virusrecom'), eval("virusrecom -h 2>&1 | grep 'Version:' | sed 's/.*Version: //' | sed 's/ (.*//'" ), topic: versions, emit: versions_virusrecom
+ tuple val("${task.process}"), val('virusrecom'), eval("virusrecom -h 2>&1 | sed -n 's/.*Version: \([^ ]*\).*/\1/p'"), topic: versions, emit: versions_virusrecom
It's shorter
------------------------------
In modules/nf-core/virusrecom/main.nf
<#11658 (comment)>:
> + input:
+ tuple val(meta), path(alignment), path(mapping)
+
+ output:
+ tuple val(meta), path("${meta.id}/"), emit: results
+ tuple val("${task.process}"), val('virusrecom'), eval("virusrecom -h 2>&1 | grep 'Version:' | sed 's/.*Version: //' | sed 's/ (.*//'" ), topic: versions, emit: versions_virusrecom
+
+ when:
+ task.ext.when == null || task.ext.when
+
+ script:
+ def args = task.ext.args ?: ''
+ def prefix = task.ext.prefix ?: "${meta.id}"
+ """
+ virusrecom \\
+ -a ${alignment} \\
Looking at the docs, we can also pass an unaligned file and have it
aligned with mafft or another tool as well as passa a iwic input.
If we were to follow the nf-core rules, we should also support this.
I believe the best approach to be is,
input:
tuple val(meta), path(fasta), path(mapping), path(iwic)
val(is_aligned)
val(alignment_tool)
script:
def args = task.ext.args ?: ''
def prefix = task.ext.prefix ?: "${meta.id}"
def use_iwic = iwic as boolean // path(iwic) will be [] when absent
def input_command = use_iwic ? "-iwic ${iwic}"
: is_aligned ? "-a ${fasta}"
: "-ua ${fasta}"
def map_command = use_iwic ? "" : "-map ${mapping}"
def at_command = (!use_iwic && !is_aligned && alignment_tool) ? "-at ${alignment_tool}" : ""
if (!use_iwic && !is_aligned && !alignment_tool) {
error "alignment_tool must be specified when input is unaligned (no iwic provided)"
}
"""
virusrecom \\
${input_command} \\
${map_command} \\
${at_command} \\
-o ${prefix} \\
-t ${task.cpus} \\
${args}
"""
------------------------------
In modules/nf-core/virusrecom/main.nf
<#11658 (comment)>:
> +
+ input:
+ tuple val(meta), path(alignment), path(mapping)
+
+ output:
+ tuple val(meta), path("${meta.id}/"), emit: results
+ tuple val("${task.process}"), val('virusrecom'), eval("virusrecom -h 2>&1 | grep 'Version:' | sed 's/.*Version: //' | sed 's/ (.*//'" ), topic: versions, emit: versions_virusrecom
+
+ when:
+ task.ext.when == null || task.ext.when
+
+ script:
+ def args = task.ext.args ?: ''
+ def prefix = task.ext.prefix ?: "${meta.id}"
+ """
+ virusrecom \\
is -q query_name a mandatory input? it seems like from the help message
that it is, if so this should also be added as an input of the module.
------------------------------
On modules/nf-core/virusrecom/meta.yml
<#11658 (comment)>:
You'll have to update this file depending on how we continue with the new
input arguments
—
Reply to this email directly, view it on GitHub
<#11658 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/BWLJZCG3QGEVY7JPAAT5TKL43LCR3AVCNFSM6AAAAACZBK2LDCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHM2DGMBYGIYDENJRGQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Author
|
Hi Joon, thank you for the detailed review! I' have tried to address all
the comments:
Note: The test data files ended up without the /data/ prefix in
test-datasets, so I used the full URL directly in the test. Let me know if
this needs to be changed.
Ready for re-review!
On Mon, May 18, 2026 at 12:06 PM Haggai Elisha ***@***.***>
wrote:
… Hello i am working on it but i may need guide. Let me send what i have and
evaluate so far on it.
Thank you
On Mon, 18 May 2026 at 11:28, Joon Klaps ***@***.***> wrote:
> ***@***.**** requested changes on this pull request.
>
> Very Nice contribution.
>
> I believe this module is going to be a bit more difficult (still totally
> feasible) to add, especially for a first module.
>
> Let me know what you think, open to discussion.
>
> If you do decide to continue, make sure all possible input combinations
> are tested & that we update the meta.yml file
> ------------------------------
>
> In modules/nf-core/virusrecom/tests/main.nf.test
> <#11658 (comment)>:
>
> > @@ -0,0 +1,29 @@
> +nextflow_process {
> + name "Test Process VIRUSRECOM"
> + script "../main.nf"
> + process "VIRUSRECOM"
> + tag "modules"
> + tag "modules_nfcore"
> + tag "virusrecom"
> +
> + test("virusrecom - fasta - stub") {
>
> Don't forget to make an actuall test once the testdata is merged in.
>
> Would also be better if the stub used these files as well instead of
> fasta & fasta.fai
> ⬇️ Suggested change
>
> - test("virusrecom - fasta - stub") {
> + test("sarscov2 - alignment [fasta] mapping [tsv] - stub") {
>
> ------------------------------
>
> In modules/nf-core/virusrecom/tests/main.nf.test
> <#11658 (comment)>:
>
> > + options "-stub"
> + when {
> + process {
> + """
> + input[0] = [
> + [ id:'test', single_end:false ],
> + file(params.modules_testdata_base_path + 'genomics/sarscov2/genome/genome.fasta', checkIfExists: true),
> + file(params.modules_testdata_base_path + 'genomics/sarscov2/genome/genome.fasta.fai', checkIfExists: true)
> + ]
> + """
> + }
> + }
> + then {
> + assert process.success
> + assertAll(
> + { assert snapshot(process.out).match() }
>
> ⬇️ Suggested change
>
> - { assert snapshot(process.out).match() }
> + { assert snapshot(sanitizeOutput(process.out)).match() }
>
> sanitizeOutput()
> <https://nf-co.re/nft-utils#sanitizeoutput---sanitize-process-output-to-create-clean-snapshots>
> makes the snapshot easier to read.
> ------------------------------
>
> In modules/nf-core/virusrecom/main.nf
> <#11658 (comment)>:
>
> > @@ -0,0 +1,38 @@
> +process VIRUSRECOM {
> + tag "$meta.id"
> + label 'process_single'
> +
> + conda "${moduleDir}/environment.yml"
> + container "${ workflow.containerEngine in ['singularity', 'apptainer'] && !task.ext.singularity_pull_docker_container ?
> + 'https://depot.galaxyproject.org/singularity/virusrecom:1.4.0--pyhdfd78af_0':
> + 'quay.io/biocontainers/virusrecom:1.4.0--pyhdfd78af_0' }"
>
> ⬇️ Suggested change
>
> - 'quay.io/biocontainers/virusrecom:1.4.0--pyhdfd78af_0' }"
> + 'biocontainers/virusrecom:1.4.0--pyhdfd78af_0' }"
>
> By default we use quay.io, users can specify their own registery if they
> want. See the docs
> <https://nf-co.re/docs/running/configuration/nextflow-for-your-system#customize-docker-registries>
> ------------------------------
>
> In modules/nf-core/virusrecom/main.nf
> <#11658 (comment)>:
>
> > @@ -0,0 +1,38 @@
> +process VIRUSRECOM {
> + tag "$meta.id"
> + label 'process_single'
>
> ⬇️ Suggested change
>
> - label 'process_single'
> + label 'process_low'
>
> We can specify the number of threads to use but we would restrict it to
> only a single thread? From their docs the default is 4. I'm not familar
> with the tool, but if it takes a long time, we might want to update to even
> process_medium
> ------------------------------
>
> In modules/nf-core/virusrecom/main.nf
> <#11658 (comment)>:
>
> > @@ -0,0 +1,38 @@
> +process VIRUSRECOM {
> + tag "$meta.id"
> + label 'process_single'
> +
> + conda "${moduleDir}/environment.yml"
> + container "${ workflow.containerEngine in ['singularity', 'apptainer'] && !task.ext.singularity_pull_docker_container ?
> + 'https://depot.galaxyproject.org/singularity/virusrecom:1.4.0--pyhdfd78af_0':
> + 'quay.io/biocontainers/virusrecom:1.4.0--pyhdfd78af_0' }"
> +
> + input:
> + tuple val(meta), path(alignment), path(mapping)
> +
> + output:
> + tuple val(meta), path("${meta.id}/"), emit: results
> + tuple val("${task.process}"), val('virusrecom'), eval("virusrecom -h 2>&1 | grep 'Version:' | sed 's/.*Version: //' | sed 's/ (.*//'" ), topic: versions, emit: versions_virusrecom
>
> ⬇️ Suggested change
>
> - tuple val("${task.process}"), val('virusrecom'), eval("virusrecom -h 2>&1 | grep 'Version:' | sed 's/.*Version: //' | sed 's/ (.*//'" ), topic: versions, emit: versions_virusrecom
> + tuple val("${task.process}"), val('virusrecom'), eval("virusrecom -h 2>&1 | sed -n 's/.*Version: \([^ ]*\).*/\1/p'"), topic: versions, emit: versions_virusrecom
>
> It's shorter
> ------------------------------
>
> In modules/nf-core/virusrecom/main.nf
> <#11658 (comment)>:
>
> > + input:
> + tuple val(meta), path(alignment), path(mapping)
> +
> + output:
> + tuple val(meta), path("${meta.id}/"), emit: results
> + tuple val("${task.process}"), val('virusrecom'), eval("virusrecom -h 2>&1 | grep 'Version:' | sed 's/.*Version: //' | sed 's/ (.*//'" ), topic: versions, emit: versions_virusrecom
> +
> + when:
> + task.ext.when == null || task.ext.when
> +
> + script:
> + def args = task.ext.args ?: ''
> + def prefix = task.ext.prefix ?: "${meta.id}"
> + """
> + virusrecom \\
> + -a ${alignment} \\
>
> Looking at the docs, we can also pass an unaligned file and have it
> aligned with mafft or another tool as well as passa a iwic input.
>
> If we were to follow the nf-core rules, we should also support this.
>
> I believe the best approach to be is,
>
> input:
> tuple val(meta), path(fasta), path(mapping), path(iwic)
> val(is_aligned)
> val(alignment_tool)
>
> script:
> def args = task.ext.args ?: ''
> def prefix = task.ext.prefix ?: "${meta.id}"
> def use_iwic = iwic as boolean // path(iwic) will be [] when absent
>
> def input_command = use_iwic ? "-iwic ${iwic}"
> : is_aligned ? "-a ${fasta}"
> : "-ua ${fasta}"
>
> def map_command = use_iwic ? "" : "-map ${mapping}"
> def at_command = (!use_iwic && !is_aligned && alignment_tool) ? "-at ${alignment_tool}" : ""
>
> if (!use_iwic && !is_aligned && !alignment_tool) {
> error "alignment_tool must be specified when input is unaligned (no iwic provided)"
> }
>
> """
> virusrecom \\
> ${input_command} \\
> ${map_command} \\
> ${at_command} \\
> -o ${prefix} \\
> -t ${task.cpus} \\
> ${args}
> """
>
> ------------------------------
>
> In modules/nf-core/virusrecom/main.nf
> <#11658 (comment)>:
>
> > +
> + input:
> + tuple val(meta), path(alignment), path(mapping)
> +
> + output:
> + tuple val(meta), path("${meta.id}/"), emit: results
> + tuple val("${task.process}"), val('virusrecom'), eval("virusrecom -h 2>&1 | grep 'Version:' | sed 's/.*Version: //' | sed 's/ (.*//'" ), topic: versions, emit: versions_virusrecom
> +
> + when:
> + task.ext.when == null || task.ext.when
> +
> + script:
> + def args = task.ext.args ?: ''
> + def prefix = task.ext.prefix ?: "${meta.id}"
> + """
> + virusrecom \\
>
> is -q query_name a mandatory input? it seems like from the help message
> that it is, if so this should also be added as an input of the module.
> ------------------------------
>
> On modules/nf-core/virusrecom/meta.yml
> <#11658 (comment)>:
>
> You'll have to update this file depending on how we continue with the new
> input arguments
>
> —
> Reply to this email directly, view it on GitHub
> <#11658 (review)>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/BWLJZCG3QGEVY7JPAAT5TKL43LCR3AVCNFSM6AAAAACZBK2LDCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHM2DGMBYGIYDENJRGQ>
> .
> Triage notifications on the go with GitHub Mobile for iOS
> <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
> or Android
> <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
>
> You are receiving this because you authored the thread.Message ID:
> ***@***.***>
>
|
Contributor
|
There are still quite a few questions unaswered:
|
Author
|
Hello Joon I have tried addressing the comments however the linter requires
the quay.io im happy to discuss that
Regards
…On Tue, May 19, 2026 at 11:20 AM Joon Klaps ***@***.***> wrote:
*Joon-Klaps* left a comment (nf-core/modules#11658)
<#11658 (comment)>
There are still quite a few questions unaswered:
- Remove quay.io from docker container
- Include more tests (non-stub tests)
- input is aligned
- input is unaligned
- input is iwic
- query name, is it a mandatory input variable? If so, we should also
add it as an input
- The label process_single is used but the tool can be multithreaded,
so this should become process_low I believe.
—
Reply to this email directly, view it on GitHub
<#11658 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/BWLJZCBBVIWGADXXYXG3CBD43QKMNAVCNFSM6AAAAACZBK2LDCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DIOBVG44DOMJTGE>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Contributor
|
Yes, that's my fault I though |
Joon-Klaps
reviewed
May 19, 2026
Contributor
Joon-Klaps
left a comment
Joon-Klaps
left a comment
There was a problem hiding this comment.
Sorry for the confusion and the long reviews.
Comment on lines
+53
to
+54
| description: Alignment tool to use for unaligned sequences (e.g. mafft, | ||
| muscle, clustalo) |
Contributor
There was a problem hiding this comment.
Suggested change
| description: Alignment tool to use for unaligned sequences (e.g. mafft, | |
| muscle, clustalo) | |
| description: Alignment tool to use for unaligned sequences (e.g. mafft, | |
| muscle, clustalo) | |
| enum: [mafft, muscle, clustalo] |
| tag "virusrecom" | ||
|
|
||
| test("sarscov2 - alignment [fasta] mapping [tsv] - stub") { | ||
| options "-stub" |
Contributor
There was a problem hiding this comment.
With this option "-stub" we don't actually run the tool virusrecom but we run the stub section of our module.
Which are these lines.
stub:
def prefix = task.ext.prefix ?: "${meta.id}"
"""
mkdir -p ${meta.id}
touch ${meta.id}/stub_output.txt
"""Typically within our tests we want to test both the script block & the stub block.
To make a test for the script block, don't include the options "-stub" in the test.
Within this test script I believe we need:
- normal test for input alignment
- normal test for unaligned input (specify one of the three tools to align)
- normal test with iwic input
- stub test with input alignment
Author
|
It's all good , I also had to read .
So do I have to adress any other thing or are we good to go?
…On Tue, May 19, 2026 at 2:05 PM Joon Klaps ***@***.***> wrote:
***@***.**** commented on this pull request.
Sorry for the confusion and the long reviews.
------------------------------
In modules/nf-core/virusrecom/meta.yml
<#11658 (comment)>:
> + description: Alignment tool to use for unaligned sequences (e.g. mafft,
+ muscle, clustalo)
⬇️ Suggested change
- description: Alignment tool to use for unaligned sequences (e.g. mafft,
- muscle, clustalo)
+ description: Alignment tool to use for unaligned sequences (e.g. mafft,
+ muscle, clustalo)
+ enum: [mafft, muscle, clustalo]
------------------------------
In modules/nf-core/virusrecom/tests/main.nf.test
<#11658 (comment)>:
> @@ -0,0 +1,58 @@
+nextflow_process {
+ name "Test Process VIRUSRECOM"
+ script "../main.nf"
+ process "VIRUSRECOM"
+ tag "modules"
+ tag "modules_nfcore"
+ tag "virusrecom"
+
+ test("sarscov2 - alignment [fasta] mapping [tsv] - stub") {
+ options "-stub"
So with this option "-stub" we don't actually run the tool virusrecom but
we run the stub section of our script.
Which are these lines.
stub:
def prefix = task.ext.prefix ?: "${meta.id}"
""" mkdir -p ${meta.id} touch ${meta.id}/stub_output.txt """
So typically within our tests we want to test both the script block & the
stub block.
To make a test for the script block, don't include the options "-stub" in
the test.
So within this test script I believe we need:
- normal test for input alignment
- normal test for unaligned input (specify one of the three tools to
align)
- normal test with iwic input
- stub test with input alignment
—
Reply to this email directly, view it on GitHub
<#11658 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/BWLJZCCP5BPZ4G7C5BIBUOL43Q5YPAVCNFSM6AAAAACZBK2LDCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHM2DGMJYGMYDONJUGY>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Contributor
|
Yes please extend the current testing framework, see my individual comments. |
Author
|
regarding the real tests,
For the aligned and iwic inputs, I can add real tests since the test data
is already in nf-core/test-datasets.
However for the unaligned input, mafft is not included in the virusrecom
biocontainer,
docker run --rm quay.io/biocontainers/virusrecom:1.4.0--pyhdfd78af_0 which
mafft
returns empty mafft not found
So the unaligned real test would fail. Should I:
Only add real tests for aligned and iwic, and keep stub for unaligned?
or Use a different container that includes both virusrecom and mafft? or
Something else?
Regards
On Tue, May 19, 2026 at 2:32 PM Haggai Elisha ***@***.***>
wrote:
… It's all good , I also had to read .
So do I have to adress any other thing or are we good to go?
On Tue, May 19, 2026 at 2:05 PM Joon Klaps ***@***.***>
wrote:
> ***@***.**** commented on this pull request.
>
> Sorry for the confusion and the long reviews.
> ------------------------------
>
> In modules/nf-core/virusrecom/meta.yml
> <#11658 (comment)>:
>
> > + description: Alignment tool to use for unaligned sequences (e.g. mafft,
> + muscle, clustalo)
>
> ⬇️ Suggested change
>
> - description: Alignment tool to use for unaligned sequences (e.g. mafft,
> - muscle, clustalo)
> + description: Alignment tool to use for unaligned sequences (e.g. mafft,
> + muscle, clustalo)
> + enum: [mafft, muscle, clustalo]
>
> ------------------------------
>
> In modules/nf-core/virusrecom/tests/main.nf.test
> <#11658 (comment)>:
>
> > @@ -0,0 +1,58 @@
> +nextflow_process {
> + name "Test Process VIRUSRECOM"
> + script "../main.nf"
> + process "VIRUSRECOM"
> + tag "modules"
> + tag "modules_nfcore"
> + tag "virusrecom"
> +
> + test("sarscov2 - alignment [fasta] mapping [tsv] - stub") {
> + options "-stub"
>
> So with this option "-stub" we don't actually run the tool virusrecom
> but we run the stub section of our script.
> Which are these lines.
>
> stub:
> def prefix = task.ext.prefix ?: "${meta.id}"
> """ mkdir -p ${meta.id} touch ${meta.id}/stub_output.txt """
>
> So typically within our tests we want to test both the script block & the
> stub block.
>
> To make a test for the script block, don't include the options "-stub"
> in the test.
>
> So within this test script I believe we need:
>
> - normal test for input alignment
> - normal test for unaligned input (specify one of the three tools to
> align)
> - normal test with iwic input
> - stub test with input alignment
>
> —
> Reply to this email directly, view it on GitHub
> <#11658 (review)>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/BWLJZCCP5BPZ4G7C5BIBUOL43Q5YPAVCNFSM6AAAAACZBK2LDCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHM2DGMJYGMYDONJUGY>
> .
> Triage notifications on the go with GitHub Mobile for iOS
> <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
> or Android
> <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
>
> You are receiving this because you authored the thread.Message ID:
> ***@***.***>
>
|
Contributor
|
We could patch the container, but let us instead just drop the funcitonality then for unaligned inputs and only allow aligned inputs or iwic files. So yeah, no need to test it. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
New module: virusrecom
Adding virusrecom — an information-theory-based method for
recombination detection of viral lineages using weighted
information content (WIC).
PR checklist