add methurator gtestimator and plot modules#11110
add methurator gtestimator and plot modules#11110edogiuili wants to merge 11 commits intonf-core:masterfrom
Conversation
famosab
left a comment
famosab
left a comment
There was a problem hiding this comment.
Thank you for your contribution to nf-core! We really appreciate it. I added a few comments to your PR.
|
|
||
| output: | ||
| tuple val(meta), path("methurator_*.yml") , emit: summary_report | ||
| path "versions.yml" , emit: versions |
There was a problem hiding this comment.
Can you update the versions output to utilize topics? More information about that can be found in the docs.
| input: | ||
| tuple val(meta), path(bam) | ||
| tuple val(meta2), path(bai) | ||
| tuple val(meta3), path(fasta) |
There was a problem hiding this comment.
Can we put all these inputs into one tuple? That will make sure you're sure that EVERY time everything comes together in the right combination.
There was a problem hiding this comment.
Since methurator uses MethylDackel inside, I sticked to the inputs that are present in MethylDackel nf-core module. Do you think it's still better to put them all into one tuple?
There was a problem hiding this comment.
Its not something I will force you to do :D but its something we try to move towards to because it has some advantages (but it will be obsolete in the future with how nextflow will change its input syntax) so I would say this is up to you and keeping it consistent to methyldackel is a good argument to keep it like this
There was a problem hiding this comment.
You can run
nextflow lint -format -sort-declarations -spaces 4 -harshil-alignmenton this file to clean this up nicely.
| """ | ||
|
|
||
| stub: | ||
| def prefix = task.ext.prefix ?: "methurator_summary_${meta.id}" |
There was a problem hiding this comment.
Why is it called _summary here and in th output just merthurator?
| Methurator is a Python package designed to estimate CpGs saturation | ||
| for DNA methylation sequencing data. |
There was a problem hiding this comment.
Please use distinct descriptions for the module and the overall tool
There was a problem hiding this comment.
See comments on above nf.test file
There was a problem hiding this comment.
You can run
nextflow lint -format -sort-declarations -spaces 4 -harshil-alignmenton this file to clean this up nicely.
|
|
||
| output: | ||
| tuple val(meta), path("plots/*.html") , emit: plots | ||
| path "versions.yml" , emit: versions |
There was a problem hiding this comment.
Can you update the versions output to utilize topics? More information about that can be found in the docs.
| def prefix = task.ext.prefix ?: "plots/${meta.id}.html" | ||
| """ | ||
| mkdir plots/ | ||
| touch ${prefix} |
There was a problem hiding this comment.
Lets not create a dir if it only contains one output
There was a problem hiding this comment.
This is necessary cause the process outputs the file into the plots/ folder, so I needed to create a file in that folder to avoid getting the following error in the stub:
Missing output file(s) `plots/*.html` expected by process `METHURATOR_PLOT (test_bam)
But maybe there is a more elegant way of doing this :D
| tuple val(meta), path(summary_report) | ||
|
|
||
| output: | ||
| tuple val(meta), path("plots/*.html") , emit: plots |
There was a problem hiding this comment.
How many *.html files do you expect?
There was a problem hiding this comment.
only 1 in this case, but it might be many if a user inputs a several BAM files simultaneously to the methurator/gtestimator and then methurator/plot. So I think it is safer to keep the *
… report with methurator_summary
|
Thanks for your suggestions Famke! I've addressed them all, apart from:
For the additional snapshots, instead of capturing the full files—which are unstable—I extracted a snapshot of the content from lines 3 to 9 for the methurator/gtestimator summary reports, and a snapshot of the HTML filename for the methurator plots. What do you think? |
|
A small fix in the eval() expression to extract the software version. I changed the expression from |
2 participants
In this PR, I add methurator gtestimator and plot modules.
PR checklist
topic: versions- See version_topicslabelnf-core modules test <MODULE> --profile dockernf-core modules test <MODULE> --profile singularitynf-core modules test <MODULE> --profile condanf-core subworkflows test <SUBWORKFLOW> --profile dockernf-core subworkflows test <SUBWORKFLOW> --profile singularitynf-core subworkflows test <SUBWORKFLOW> --profile conda