Conversation
…tations, restructure subworkflows
Add gapseq media feature, update docs, and enhance pipeline visualiza…
Fix metabolic modeling defaults and resolve technical issues
…pipeline-specific config
Preparations for the first release
fixed version issue with summary, fixed conda issue for carveme
Normalize Python version to major.minor only
add possibilities to customize gapseq, fixed issue with multiple db downloads
adressing the comments
Addressing cooments
|
Warning Newer version of the nf-core template is available. Your pipeline is using an old version of the nf-core template: 4.0.2. For more documentation on how to update your pipeline, please see the nf-core documentation and Synchronisation documentation. |
Automated security rollout could not apply this cleanly. Resolve the remaining conflict markers and *.rej files, then open a PR from 'patch' into 'dev'.
Remove vulnerable PR-comment artifact pattern
modified capruting python version to major.minor style
erikrikarddaniel
left a comment
erikrikarddaniel
left a comment
There was a problem hiding this comment.
Looks good. I have a few discretionary comments and one that I would like to see a good argument for not changing before approving: The param used to decide which type of Bakta database to download.
| // TODO nf-core: Give any required params for the test so that command line flags are not needed | ||
| input = params.pipelines_testdata_base_path + 'viralrecon/samplesheet/samplesheet_test_illumina_amplicon.csv' | ||
| // Input data - small bacterial genome test dataset | ||
| input = "$projectDir/assets/samplesheet.csv" |
There was a problem hiding this comment.
Why haven't you uploaded this to the testdata repo? I see the assets file has an https path so should work I suppose. I don't know the actual rules; I always uploaded to the repo.
There was a problem hiding this comment.
Nice logo. I think I would move the text a bit to the left to have less space between the drawing and the text, and reduce space on the sides. Totally up to you.
| // Use provided database path | ||
| ch_baktadb = Channel.fromPath(options.baktadb, checkIfExists: true) | ||
| } else { | ||
| error "Bakta requires a database. Please provide --baktadb /path/to/db or use --baktadb_download true" |
There was a problem hiding this comment.
A technique we use in magmap and metatdenovo is to autodetect the presence of a database and only have a parameter for the name of the directory. If you use the storeDir directive to point to the director, e.g.: storeDir = { "${params.baktadb}" } Nextflow will notice whether the files are already there, in which case execution of the download module will be skipped. With a default for the path, the user basically doesn't have to input anything except maybe a --skip_bakta param if they want to skip. Very handy IMO, but I know others are skeptical.
I see below that there are other places where this pattern fits.
There was a problem hiding this comment.
Thanks for pointing this out! This is indeed more practical. I will adjust it not only for bakta, but for macsyfinder and traitar as well
| // | ||
| // Create channel from samplesheet (already formatted by PIPELINE_INITIALISATION) | ||
| // | ||
| ch_genomes = ch_samplesheet |
There was a problem hiding this comment.
Why rename it here rather than have the argument to the workflow get the correct name right away, i.e. line 20?
There was a problem hiding this comment.
Unusual to see so much content here for an unreleased pipeline -- what are the changes, fixes etc. relative to?
| annotation_tool = 'prokka' // 'prokka' or 'bakta' | ||
| baktadb = null // Path to Bakta database (required if annotation_tool='bakta') | ||
| baktadb_download = false // Download Bakta database automatically | ||
| baktadb_download_args = '--type light' // Arguments for Bakta database download |
There was a problem hiding this comment.
Looks a bit odd to have a verbatim param here. I'd rather have baktadb_type = 'light' and specify all alternatives in nextflow_schema.json and let users add whatever other parameters via the ext.args mechanism in their own config file.
| }, | ||
| "baktadb_download_args": { | ||
| "type": "string", | ||
| "default": "--type light", |
There was a problem hiding this comment.
See comment in nextflow.config.
| <!-- TODO nf-core: Add bibliography of tools and data used in your pipeline --> | ||
| If you use nf-core/bacmodel for your analysis, please cite the pipeline along with the tools it uses: | ||
|
|
||
| ### Core annotation tools |
There was a problem hiding this comment.
Aren't we supposed to have this in CITATIONS.md instead?
Added possibility for user to specify already downloaded db
Merge pull request #2 from nf-core/dev
4 participants
Do not merge! This is a PR of
devcompared to theTEMPLATEbranch for whole-pipeline reviewing purposes.Changes should be made to
dev(wiill also update this PR) and this PR should not be merged! The actual release PR is at #4