-
Notifications
You must be signed in to change notification settings - Fork 959
Feature/localcdsearch #9632
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
Feature/localcdsearch #9632
Conversation
jfy133
left a comment
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.
Review for just download mostly what we talked about on Slack and then a few other minor things
Co-authored-by: James A. Fellows Yates <jfy133@gmail.com>
Co-authored-by: James A. Fellows Yates <jfy133@gmail.com>
Co-authored-by: James A. Fellows Yates <jfy133@gmail.com>
Co-authored-by: James A. Fellows Yates <jfy133@gmail.com>
Co-authored-by: James A. Fellows Yates <jfy133@gmail.com>
Co-authored-by: James A. Fellows Yates <jfy133@gmail.com>
Co-authored-by: James A. Fellows Yates <jfy133@gmail.com>
Co-authored-by: James A. Fellows Yates <jfy133@gmail.com>
Co-authored-by: James A. Fellows Yates <jfy133@gmail.com>
Co-authored-by: James A. Fellows Yates <jfy133@gmail.com>
vagkaratzas
left a comment
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.
Nice! Couple of things and it should be getting there
| 'biocontainers/local-cd-search:0.3.0--pyhdfd78af_0' }" | ||
|
|
||
| input: | ||
| val databases |
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.
If you ask me, I would add a meta since it's the only input, but don't really have a strong opinion for or against it. mmseqs_databases module is also not using any meta for val database which is also a single input.
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.
Meta should be the database name, right? The problem with meta here is that the input is a list that can contain multiple databases. It sound to me that handling a list on meta will be problematic. The option would be to join the list into a string, but that would be hard to read
| process { | ||
| """ | ||
| input[0] = [ [id:'test'], file(params.modules_testdata_base_path + 'genomics/sarscov2/genome/proteome.fasta', checkIfExists: true) ] | ||
| input[1] = LOCALCDSEARCH_DOWNLOAD.out.db |
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 curious, if you had downloaded multiple dbs, would this still be the same? Or do you need to select one or more of the downloaded dbs somehow?
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.
The downloader tool creates the databases directory structure and the annotator detects all the available databases in the given directory. Then the tool runs the annotation versus ALL the detected databases. There's no way to select which database to use when multiple databases exists
Updated version information to use a variable for better maintainability.
Added version information for the tool in the main.nf file.
Added version information to the main.nf file.
Co-authored-by: Evangelos Karatzas <32259775+vagkaratzas@users.noreply.github.com>
vagkaratzas
left a comment
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.
Two last comments and I think it's good to go
vagkaratzas
left a comment
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 IMO! Can always patch-update later if something's off!
* local-cd-search annotate and download modules * Fixing lint * Update localcdsearch tool metadata in meta.yml * Fix container URL selection logic in main.nf * Fix container URL for local-cd-search * Fix container URL for local-cd-search * Fix container URL for local-cd-search * Fix container URL for local-cd-search * Fix container URL for local-cd-search * Update modules/nf-core/localcdsearch/annotate/main.nf Co-authored-by: James A. Fellows Yates <jfy133@gmail.com> * Update modules/nf-core/localcdsearch/download/main.nf Co-authored-by: James A. Fellows Yates <jfy133@gmail.com> * Update modules/nf-core/localcdsearch/download/main.nf Co-authored-by: James A. Fellows Yates <jfy133@gmail.com> * Update modules/nf-core/localcdsearch/download/main.nf Co-authored-by: James A. Fellows Yates <jfy133@gmail.com> * Update modules/nf-core/localcdsearch/download/main.nf Co-authored-by: James A. Fellows Yates <jfy133@gmail.com> * Update modules/nf-core/localcdsearch/download/main.nf Co-authored-by: James A. Fellows Yates <jfy133@gmail.com> * Update modules/nf-core/localcdsearch/download/main.nf Co-authored-by: James A. Fellows Yates <jfy133@gmail.com> * Update modules/nf-core/localcdsearch/download/meta.yml Co-authored-by: James A. Fellows Yates <jfy133@gmail.com> * Update modules/nf-core/localcdsearch/download/meta.yml Co-authored-by: James A. Fellows Yates <jfy133@gmail.com> * Update modules/nf-core/localcdsearch/download/meta.yml Co-authored-by: James A. Fellows Yates <jfy133@gmail.com> * Singularity container added * edamontology added * Updating test for one input only * Updating snap * Updating test for db downloding * Update version handling in main.nf * Update version expression in meta.yml * Refactor version handling in main.nf Updated version information to use a variable for better maintainability. * Update version retrieval expression in meta.yml * Add version information to stub Added version information for the tool in the main.nf file. * Add version information to stub Added version information to the main.nf file. * Update modules/nf-core/localcdsearch/download/meta.yml Co-authored-by: Evangelos Karatzas <32259775+vagkaratzas@users.noreply.github.com> * Refactor assertions to include process success check * Refactor assertions for process success in tests * Update container URL for local-cd-search * Remove redundant success assertion in tests --------- Co-authored-by: James A. Fellows Yates <jfy133@gmail.com> Co-authored-by: Evangelos Karatzas <32259775+vagkaratzas@users.noreply.github.com>
* local-cd-search annotate and download modules * Fixing lint * Update localcdsearch tool metadata in meta.yml * Fix container URL selection logic in main.nf * Fix container URL for local-cd-search * Fix container URL for local-cd-search * Fix container URL for local-cd-search * Fix container URL for local-cd-search * Fix container URL for local-cd-search * Update modules/nf-core/localcdsearch/annotate/main.nf Co-authored-by: James A. Fellows Yates <jfy133@gmail.com> * Update modules/nf-core/localcdsearch/download/main.nf Co-authored-by: James A. Fellows Yates <jfy133@gmail.com> * Update modules/nf-core/localcdsearch/download/main.nf Co-authored-by: James A. Fellows Yates <jfy133@gmail.com> * Update modules/nf-core/localcdsearch/download/main.nf Co-authored-by: James A. Fellows Yates <jfy133@gmail.com> * Update modules/nf-core/localcdsearch/download/main.nf Co-authored-by: James A. Fellows Yates <jfy133@gmail.com> * Update modules/nf-core/localcdsearch/download/main.nf Co-authored-by: James A. Fellows Yates <jfy133@gmail.com> * Update modules/nf-core/localcdsearch/download/main.nf Co-authored-by: James A. Fellows Yates <jfy133@gmail.com> * Update modules/nf-core/localcdsearch/download/meta.yml Co-authored-by: James A. Fellows Yates <jfy133@gmail.com> * Update modules/nf-core/localcdsearch/download/meta.yml Co-authored-by: James A. Fellows Yates <jfy133@gmail.com> * Update modules/nf-core/localcdsearch/download/meta.yml Co-authored-by: James A. Fellows Yates <jfy133@gmail.com> * Singularity container added * edamontology added * Updating test for one input only * Updating snap * Updating test for db downloding * Update version handling in main.nf * Update version expression in meta.yml * Refactor version handling in main.nf Updated version information to use a variable for better maintainability. * Update version retrieval expression in meta.yml * Add version information to stub Added version information for the tool in the main.nf file. * Add version information to stub Added version information to the main.nf file. * Update modules/nf-core/localcdsearch/download/meta.yml Co-authored-by: Evangelos Karatzas <32259775+vagkaratzas@users.noreply.github.com> * Refactor assertions to include process success check * Refactor assertions for process success in tests * Update container URL for local-cd-search * Remove redundant success assertion in tests --------- Co-authored-by: James A. Fellows Yates <jfy133@gmail.com> Co-authored-by: Evangelos Karatzas <32259775+vagkaratzas@users.noreply.github.com>
This PR contains the addition of the local-cd-search tool module (annotate and download). Please note that the tool version is not displayed in v0.3.0. I had opened an issue requesting the functionality to the original author. At the moment, the version is hardcoded.
Also, local-cd-search image is not available in Seqera Containers yet and Galaxy Depot doesn't have it either, so we are relying on biocontainers.
Thank you for reviewing!
PR checklist
Closes #9574
topic: versions- See version_topicslabelnf-core modules test <MODULE> --profile dockernf-core modules test <MODULE> --profile singularitynf-core modules test <MODULE> --profile conda