-
Notifications
You must be signed in to change notification settings - Fork 34
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
Update modules #449
Update modules #449
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, just a comment about the names of the channel branches. It took me a couple of minutes to figure out which branch was what. Or maybe that is as easy as it will get. Also, the svdb_query prefix which should not cause any issues for now but is a potential headache.
@@ -13,7 +13,8 @@ process SVDB_QUERY { | |||
val(in_frqs) | |||
val(out_occs) | |||
val(out_frqs) | |||
path (vcf_dbs) | |||
path(vcf_dbs) | |||
path(bedpe_dbs) | |||
|
|||
output: | |||
tuple val(meta), path("*_query.vcf") , emit: vcf |
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.
Just realising that this module is dependent on the user/developer setting ext.prefix
to something like `${meta.id}_svdbquery. Failing to do this it won't work. Either the module needs to be hardcoded to add that prefix or the output needs to change to use ${prefix}.vcf. Or have I missed something?
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.
SVDB's output carries that suffix by default https://github.com/J35P312/SVDB/blob/c8c538089fd5f05d891db3686d83a79c97d5adbf/svdb/__main__.py#L32
Co-authored-by: Anders Jemt <[email protected]>
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, I will need to look into the new module tests
@@ -4,7 +4,7 @@ process EKLIPSE { | |||
label 'process_single' | |||
|
|||
// WARN: Version information not provided by tool on CLI. Please update this string when bumping container versions. | |||
conda "bioconda::eklipse=1.8" | |||
conda "${moduleDir}/environment.yml" |
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.
Very nice, now the environment is directly there. Is it like that for all the new modules?
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.
Yupe.. with nf-tests every tool and subtool will come as a unit containing scripts, tests, and the environment :)
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! Could you just check that SVDB_query works as intended when you run both a vcf and bedpe database. Right now they will have the same suffix and I don't know which file be selected to the emit channel. Go ahead and merge when you feel confident in the code
…edisease into smallupdates
PR checklist
This PR updates two modules (eklipse & svdb query) and introduces along relevant changes in the pipeline.
nf-core lint
).nextflow run . -profile test,docker --outdir <OUTDIR>
).nextflow run . -profile test_one_sample,docker --outdir <OUTDIR>
).docs/usage.md
is updated.docs/output.md
is updated.CHANGELOG.md
is updated.README.md
is updated (including new tool citations and authors/contributors).