Adding option for setting file-output-format for VEP #582
Conversation
|
I think it's a good proof of concept. |
@maxulysse : I changed the solution so that the vep-output-format is set through The vep-output-format could be set through The thing is that, as far as I can see, I need to get a "access" (i.e. If you or someone else have suggestions for a better solution, I'm all ears. Cheers |
| --vcf \\ | ||
| --stats_file ${prefix}.summary.html | ||
| --stats_file ${prefix}.summary.html \\ | ||
| --$vep_output |
There was a problem hiding this comment.
For this parameter I would almost say it should be set with $args . That way the module output can be configured via the modules.config and can also eaily be overwritten by the user, even if we don't parameterize it. (In sarek I would propose an additional parameter though: --vep_out_format defaulting to vcf
|
Love this version, you can go open a PR in modules ;-) |
|
Shot! I just discovered that my addition of the CLI-option Any suggestion on how to handle this? |
|
yes by running |
Many thanks. I just discovered the |
|
yes don't try annotating it by hand. the tool opens the file up in the web browser so you than sink a lot of time in finding the perfect icon for the parameter 😆 |
|
can you install vep through |
I'll give it a try. @nvnieuwk and I just released a new version of the vep-module in |
|
@nf-core-bot fix linting |
|
conf/modules.config
Outdated
| withName: 'NFCORE_SAREK:SAREK:ANNOTATE:ANNOTATION_ENSEMBLVEP:ENSEMBLVEP' { | ||
| ext.prefix = {"${meta.id}_VEP"} | ||
| } | ||
|
|
||
| withName: ".*:ANNOTATION_MERGE:ENSEMBLVEP" { | ||
| ext.prefix = {"${meta.id}_snpEff"} |
There was a problem hiding this comment.
| ext.prefix = {"${meta.id}_snpEff"} | |
| ext.prefix = {"${meta.id}_snpEff_VEP"} |
I think we need to add _VEP here too.
Can you check when producing annotated vcf with --tools merge if the _VEP is there?
|
@asp8200 I'm sorry it looks like we merged a PR in |
Co-authored-by: Maxime U. Garcia <maxime.garcia@scilifelab.se>
asp8200
changed the title
There was a problem hiding this comment.
I looked through all the changes. Nice work @asp8200 ! Seems like they are well-coordinated with the others as well, so here's your approve 👍
| @@ -848,6 +848,7 @@ workflow SAREK { | |||
| if (params.tools.contains('merge') || params.tools.contains('snpeff') || params.tools.contains('vep')) { | |||
|
|
|||
| ANNOTATE(vcf_to_annotate, | |||
| fasta, | |||
There was a problem hiding this comment.
Hi! I guess a fasta is required for some of the new output formats? Is it still possible to run the annotation step without a fasta for the default vcf output?
There was a problem hiding this comment.
The FASTA is sometimes needed for plugins, but it should be okay for most runs to do it without a FASTA
There was a problem hiding this comment.
Sure, I just mean, does this step still run in the sarek context :) Did you test this?
There was a problem hiding this comment.
Oh no I don't know in the context of Sarek, but it was running without fasta before right?
There was a problem hiding this comment.
fasta is fully optionnal, but fasta is the only mandatory in sarek, so we can actually use it always.
Do we want not to use it by default?
Should we add a params to include it or not like --vep_include_fasta or --vep_no_fasta, and from that, it'll be easy to populate the fasta channel or not within the sarek workflow
There was a problem hiding this comment.
sure for me it is fine to have fasta as fully mandatory, should just communicate it :) Don't need to overcomplicate it now
| build_push "GRCm39" "mus_musculus" "104" "104.3" | ||
| build_push "CanFam3.1" "canis_lupus_familiaris" "104" "104.3" | ||
| build_push "WBcel235" "caenorhabditis_elegans" "104" "104.3" | ||
| build_push "GRCh37" "homo_sapiens" "105" "105.0" |
There was a problem hiding this comment.
@maxulysse should the igenomes.config be updated here as well? or separate PR?
There was a problem hiding this comment.
Oooh, good point, I forgot about it, yes, it should be done here
There was a problem hiding this comment.
@maxulysse and @FriederikeHanssen : I don't understand what you guys are talking about regarding igenomes.config 😆 Does this PR need to contain updates to conf/igenomes.config ? If so, maybe you could include them? Cheers
There was a problem hiding this comment.
@asp8200 no worries then, I noticed that I need to add some fixes to the VEP modules anyway, so I can include that in a future PR
|
@asp8200 Can you just update the CHANGELOG and we're good to go |
Adding new CLI-option
--vep_out_formatfor setting file-output-format for VEP. Possible valuesjson,tabandvcf. (Defaults tovcf.)Closes #575.
This solution uses a new version of the ensemblvep-module from nf-core/modes (nf-core/modules#1775).
The VEP-output-files get names like
PR checklist
scrape_software_versions.pynf-core lint .).nextflow run . -profile test,docker).docs/usage.mdis updated.docs/output.mdis updated.CHANGELOG.mdis updated.README.mdis updated (including new tool citations and authors/contributors).