Big Bakta update #235
Merged
Big Bakta update #235
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
louperelo
reviewed
Mar 6, 2023
nextflow.config
Outdated
| @@ -27,23 +27,25 @@ params { | |||
| annotation_prodigal_forcenonsd = false | |||
|
|
|||
| annotation_bakta_db = null | |||
| annotation_bakta_db_light = null | |||
There was a problem hiding this comment.
I am a bit confused about the new parameter:
--annotation_bakta_db expects a path to a local DB (might be full or light, depends on what the user downloaded himself)
while --annotation_bakta_db_light determines in the modules.config, if in BAKTA_BAKTADBDOWNLOAD the ext.args --type will be set to full or light:
params.annotation_bakta_db_light ? '--type light' : '--type full'
Shouldn't this then be a boolean parameter with default false instead of null? If no path to an existing baktaDB ist given via --annotation_bakta_db, then the full bakta_db is downloaded unless annotation_bakta_db_light was set to true.
Do we want the light or the full version to be default when downloading?
There was a problem hiding this comment.
Ahh yes, that's why reviews are important :D The new parameter should of course be boolean.
I remember my reason for having null was that there was an issue with nf-core schema build when putting false as default, but forgot to check/fix that.
Default: I would go for full DB, but can test if there are big differences to the light DB in the output. If not, let's use light of course.
There was a problem hiding this comment.
Maybe consider renaming --annotation_bakta_db to make clear that this is for a local installation. Something like --annotation_bakta_db_path or --annotation_bakta_db_local?
I agree with the full db as default. Then the parameter --annotation_bakta_db_light can just stay as it is.
There was a problem hiding this comment.
I agree, we should disambiguate, and default should be set to a boolean not null. Another suggestion:
--annotation_bakta_downloadlightdb
--annotation_bakta_localdbpath
Or something?
jfy133
requested changes
Mar 8, 2023
nextflow.config
Outdated
| @@ -27,23 +27,25 @@ params { | |||
| annotation_prodigal_forcenonsd = false | |||
|
|
|||
| annotation_bakta_db = null | |||
| annotation_bakta_db_light = null | |||
There was a problem hiding this comment.
I agree, we should disambiguate, and default should be set to a boolean not null. Another suggestion:
--annotation_bakta_downloadlightdb
--annotation_bakta_localdbpath
Or something?
This was
linked to
issues
Mar 8, 2023
louperelo
approved these changes
Mar 9, 2023
jfy133
requested changes
Mar 10, 2023
There was a problem hiding this comment.
I agree with Lousia, I find the current parameters too similar in the naming scheme, which I Worry may confuse people that they are for related functions (e.g. maybe _local means download the full database, as it has the same camel case structure as indicating the download type).
Alternatively, we could replace the boolean for the light with a string:
--annotation_bakta_db_localpath '/path/to/'
--annotation_bakta_db_downloadtype 'light' ## where default is 'full'
Other than the naming, I think the code looks fine 👍
|
@nf-core-bot fix linting |
jfy133
approved these changes
Mar 10, 2023
Co-authored-by: James A. Fellows Yates <[email protected]>
This was referenced Mar 27, 2023
9 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
params.annotation_bakta_db_lightto be able to switch between the light (1.3 GB) and full (33.1 GB) version of the Bakta DBCloses #234
Closes #214
PR checklist
nf-core lint).nextflow run . -profile test,docker --outdir <OUTDIR>).docs/usage.mdis updated.docs/output.mdis updated.CHANGELOG.mdis updated.README.mdis updated (including new tool citations and authors/contributors).