-
Notifications
You must be signed in to change notification settings - Fork 658
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
New module : Pilon #3331
New module : Pilon #3331
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
Formatting fails and I have a few suggestions, but fine overall I think.
Co-authored-by: Daniel Straub <[email protected]>
Co-authored-by: Daniel Straub <[email protected]>
tuple val(meta), path(fasta) | ||
tuple val(meta_bam), path(bam), path(bai) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because fasta, bam and bai have to come all from the same sample, would it be rather:
tuple val(meta), path(fasta) | |
tuple val(meta_bam), path(bam), path(bai) | |
tuple val(meta), path(fasta), path(bam), path(bai) |
I am not sure, thats just a question because I do not know better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is an interesting suggestion. Both would work.
I prefer to have them split, and it works if meta and meta_bam are the same.
In the case of purge_dups, they concatenated all the inputs and I had to split them because sometimes my meta reflects the input sequencing type (SR, PacBio, ONT), not only the sample name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer to have them split, and it works if meta and meta_bam are the same.
Hm ok, I would prefer having them not split. Because I think the typical application is that before the module is some channel magic that joins the required input (genome = fasta, alignment = bam & bai) to have matching files by ensuring that meta is matching.
In the case of purge_dups, they concatenated all the inputs and I had to split them because sometimes my meta reflects the input sequencing type (SR, PacBio, ONT), not only the sample name.
That seems to me rather the exception than the usual module output. But I might be wrong.
Unfortunately the nf-core modules guidelines do not specify this clearly, see https://nf-co.re/developers/modules#inputoutput-options, i.e. Directly associated auxiliary files to an input file MAY be defined within the same input channel alongside the main input channel (e.g. BAM and BAI).
, so it seems I shouldn't insist on it!
Co-authored-by: Daniel Straub <[email protected]>
Co-authored-by: Daniel Straub <[email protected]>
Co-authored-by: Daniel Straub <[email protected]>
tuple val(meta), path(fasta) | ||
tuple val(meta_bam), path(bam), path(bai) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer to have them split, and it works if meta and meta_bam are the same.
Hm ok, I would prefer having them not split. Because I think the typical application is that before the module is some channel magic that joins the required input (genome = fasta, alignment = bam & bai) to have matching files by ensuring that meta is matching.
In the case of purge_dups, they concatenated all the inputs and I had to split them because sometimes my meta reflects the input sequencing type (SR, PacBio, ONT), not only the sample name.
That seems to me rather the exception than the usual module output. But I might be wrong.
Unfortunately the nf-core modules guidelines do not specify this clearly, see https://nf-co.re/developers/modules#inputoutput-options, i.e. Directly associated auxiliary files to an input file MAY be defined within the same input channel alongside the main input channel (e.g. BAM and BAI).
, so it seems I shouldn't insist on it!
* master: New module : Pilon (nf-core#3331) Islandpath (nf-core#3145)
PR checklist
Closes #831
versions.yml
file.label
PROFILE=docker pytest --tag <MODULE> --symlink --keep-workflow-wd --git-aware
PROFILE=singularity pytest --tag <MODULE> --symlink --keep-workflow-wd --git-aware
PROFILE=conda pytest --tag <MODULE> --symlink --keep-workflow-wd --git-aware