-
Notifications
You must be signed in to change notification settings - Fork 443
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
wrappers for ppanggolin all and ppanggolin msa #6645
base: main
Are you sure you want to change the base?
Conversation
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.
Wonderful. Added a few comments.
tools/ppanggolin/.shed.yml
Outdated
homepage_url: https://ppanggolin.readthedocs.io/en/latest/ | ||
long_description: | | ||
Depicting microbial species diversity via a Partitioned PanGenome Graph Of Linked Neighbors. | ||
remote_repository_url: https://github.com/labgem/PPanGGOLiN |
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.
I think this should point to the IUC repo. Use this one for homepage_url
. The readthedocs URL might be cool to mention in the tool's help section.
remote_repository_url: https://github.com/labgem/PPanGGOLiN | ||
type: unrestricted | ||
categories: | ||
- Genome annotation |
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.
Is this a good category for the tool?
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.
I think so but I am not sure. Roary (a tool with overlapping functionalities) is classified in "Genomics Analysis -> Annotation" in https://usegalaxy.fr/ and in "Genome Analysis Tools -> Genome annotation" in https://galaxy.migale.inrae.fr/. Is there a list of categories and/or a guideline to choose the right category for the tool ?
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.
It's just the toolshed categories. You can see all of them here: https://toolshed.g2.bx.psu.edu/
tools/ppanggolin/ppanggolin_all.xml
Outdated
@@ -0,0 +1,336 @@ | |||
<tool id="ppanggolin_all" name="PPanGGOLiN all" version="@TOOL_VERSION@+galaxy@VERSION_SUFFIX@" profile="21.05"> |
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.
Maybe a newer profile, e.g. 23.0?
tools/ppanggolin/ppanggolin_all.xml
Outdated
@@ -0,0 +1,336 @@ | |||
<tool id="ppanggolin_all" name="PPanGGOLiN all" version="@TOOL_VERSION@+galaxy@VERSION_SUFFIX@" profile="21.05"> | |||
<description>generates a partitioned pangenome graph with predicted RGP, spots and modules.</description> |
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.
- Remove the
.
- Can you shorten this as much as possible? The
tool name: description
will be shown in Galaxy's tool menu.
tools/ppanggolin/ppanggolin_all.xml
Outdated
echo -n > "./tmp_ppanggolin/organism_list/organism.list" && | ||
|
||
#for $i, $file in enumerate($input_type.genomes): | ||
#set base_name = str($file.name).split('.')[0] |
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.
$file.element_identifier
is preferred instead of $file.name
tools/ppanggolin/ppanggolin_all.xml
Outdated
<conditional name="input_nb_of_partitions"> | ||
<param name="choice_nb_of_partitions" type="select" label="Number of partitions"> | ||
<option value="automatic" selected="true">Automatic</option> | ||
<option value="manually_set_to">Manually set to:</option> | ||
</param> | ||
<when value="automatic"/> | ||
<when value="manually_set_to"> | ||
<param argument="-K" name="nb_of_partitions" type="integer" value="3" min="2" max="20" label="Number of partitions"/> | ||
</when> | ||
</conditional> |
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.
Also here, you could make nb_of_partitions
an optional parameter instead and document that empty means automatic and 3 as a good choice.
tools/ppanggolin/ppanggolin_all.xml
Outdated
<option value="32">32 - Balanophoraceae Plastid</option> | ||
<option value="33">33 - Cephalodiscidae Mitochondrial</option> | ||
</param> | ||
<param name="do_defrag" type="boolean" checked="true" label="Gene family defragmentation"/> |
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.
You can add truevalue="" falsevalue="--no_defrag"
and just use $do_defrag
in the command section, i.e. remove the #if
.
tools/ppanggolin/ppanggolin_all.xml
Outdated
</param> | ||
<param name="do_defrag" type="boolean" checked="true" label="Gene family defragmentation"/> | ||
|
||
<section name="basic_pangenome_output_files" title="Basic pangenome output files" expanded="False"> |
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.
I would not add sections for single parameters. The space used for expanded="False"
should be about the same for a single parameter. Maybe one "advanced parameters" section for all of them?
tools/ppanggolin/ppanggolin_msa.xml
Outdated
mkdir -p "./tmp_ppanggolin/tmpdir_msa" && | ||
|
||
ppanggolin msa | ||
--pangenome $pangenome_h5 |
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.
Add single quotes (always for data
and text
inputs as well as output files).
tools/ppanggolin/ppanggolin_msa.xml
Outdated
<option value="dna">DNA</option> | ||
</param> | ||
|
||
<param argument="--phylo" name="do_phylo" type="boolean" checked="true" label="Writes a whole genome msa file for additional phylogenetic analysis (recommended)"/> |
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.
truevalue/falsevalue for all booleans?
Thanks @bernt-matthias , I implemented your suggestions, does it look good to you ? I couldn't get the validator to work for checking the consistency of the input files format, I didn't find a way to loop through multiple datasets or dataset collection and didn't find an example of such a validator. I implemented this check in the section by raising an exception. Best, Thomas |
FOR CONTRIBUTOR: