add new tool tiara #1428
add new tool tiara #1428
Conversation
|
Tests are unfortunately failng. |
tools/tiara/macros.xml
Outdated
| <requirements> | ||
| <requirement type="package" version="@TOOL_VERSION@">tiara</requirement> | ||
| <requirement type="package" version="3.8">python</requirement> | ||
| <yield/> |
There was a problem hiding this comment.
I think <yield> is just misplaced here?
| #if $cutoff_stage1 | ||
| -p $cutoff_stage1 | ||
| #if $cutoff_stage2 | ||
| $cutoff_stage2 | ||
| #end if | ||
| #end if |
There was a problem hiding this comment.
Is nested if statement done purposefully here?
tools/tiara/tiara.xml
Outdated
| <param name="first_stage_kmer" type="select" label="Select k-mer length used in the first stage of classification (Default: 6)."> | ||
| <option value="4">k-mer length 4</option> | ||
| <option value="5">k-mer length 5</option> | ||
| <option value="6">default k-mer length</option> |
There was a problem hiding this comment.
| <option value="6">default k-mer length</option> | |
| <option value="6" selected="True">default k-mer length</option> |
tools/tiara/tiara.xml
Outdated
| <option value="4">k-mer length 4</option> | ||
| <option value="5">k-mer length 5</option> | ||
| <option value="6">k-mer length 6</option> | ||
| <option value="7">default k-mer length</option> |
There was a problem hiding this comment.
| <option value="7">default k-mer length</option> | |
| <option value="7" selected="True">default k-mer length</option> |
tools/tiara/tiara.xml
Outdated
|
|
||
| The tiara is a Deep-learning-based approach for identification of eukaryotic sequences in the metagenomic data powered by PyTorch. | ||
|
|
There was a problem hiding this comment.
Maybe a bit more text about required inputs and generated outputs?
tools/tiara/tiara.xml
Outdated
| <option value="pro">prokarya</option> | ||
| <option value="all">all</option> | ||
| </param> | ||
| <param name="probabilities" type="boolean" truevalue="--pr" falsevalue="" checked="false" label="Add probabilities of individual classes for each sequence."/> |
There was a problem hiding this comment.
Maybe changing name to argument is better in this case? This then needs to be changed every place that has an argument.
You can check this wrapper for reference: https://github.com/galaxyproject/tools-iuc/blob/main/tools/pairtools/parse.xml#L33
There was a problem hiding this comment.
Hi @Minamehr,
I've had a quick look at the PR and overall looks good. I've commented some of the changes that would be better to incorporate.
Best!
Co-authored-by: Saim Momin <64724322+SaimMomin12@users.noreply.github.com>
Co-authored-by: Saim Momin <64724322+SaimMomin12@users.noreply.github.com>
Co-authored-by: Saim Momin <64724322+SaimMomin12@users.noreply.github.com>
Co-authored-by: Saim Momin <64724322+SaimMomin12@users.noreply.github.com>
Co-authored-by: Saim Momin <64724322+SaimMomin12@users.noreply.github.com>
Co-authored-by: Saim Momin <64724322+SaimMomin12@users.noreply.github.com>
Co-authored-by: Saim Momin <64724322+SaimMomin12@users.noreply.github.com>
tools/tiara/tiara.xml
Outdated
| -p $cutoff_stage1 | ||
| #if $cutoff_stage2 | ||
| $cutoff_stage2 |
There was a problem hiding this comment.
Assuming it to be a nested if loop 👍
tools/tiara/tiara.xml
Outdated
| <option value="all">all</option> | ||
| </param> | ||
| <param argument="probabilities" type="boolean" truevalue="--pr" falsevalue="" checked="false" label="Add probabilities of individual classes for each sequence."/> | ||
| <param argument="verbose" type="boolean" truevalue="-v" falsevalue="" checked="false" label="Display some additional messages and progress bar during classification."/> |
There was a problem hiding this comment.
verbose is usually a parameter that should not be exposed to a user I think. You could enable it by default for example or not.
tools/tiara/tiara.xml
Outdated
| </param> | ||
| <param argument="probabilities" type="boolean" truevalue="--pr" falsevalue="" checked="false" label="Add probabilities of individual classes for each sequence."/> | ||
| <param argument="verbose" type="boolean" truevalue="-v" falsevalue="" checked="false" label="Display some additional messages and progress bar during classification."/> | ||
| <param argument="min_len" type="integer" value="3000" optional="true" label="Minimum length of a sequence. Default: 3000 bp." help="Specify the desired minimum length in base pairs.Default value is 3000 bp and we do not recommend classifying sequences shorter than 1000 bp. "/> |
There was a problem hiding this comment.
please consider adding min / max wherever possible.
There was a problem hiding this comment.
@Minamehr there are many more where you can add min/max ;)
|
Thanks @Minamehr! |
|
Please do now number 3 here: https://usegalaxy-eu.github.io/posts/2020/08/22/three-steps-to-galaxify-your-tool/ |
Hi,
This pull request aims to add a new tool, Tiara. It is a deep-learning-based approach for identifying eukaryotic sequences in metagenomic data.
Kind regards,
Mina