Skip to content

update raxml-ng to 2.0.0 + UI refactor#7829

Open
amkozlov wants to merge 17 commits intogalaxyproject:mainfrom
amkozlov:raxmlng-wrapper
Open

update raxml-ng to 2.0.0 + UI refactor#7829
amkozlov wants to merge 17 commits intogalaxyproject:mainfrom
amkozlov:raxmlng-wrapper

Conversation

@amkozlov
Copy link
Copy Markdown
Contributor

FOR CONTRIBUTOR:

  • I have read the CONTRIBUTING.md document and this tool is appropriate for the tools-iuc repo.
  • License permits unrestricted use (educational + commercial)
  • This PR adds a new tool or tool collection
  • This PR updates an existing tool or tool collection
  • This PR does something else (explain below)

Comment thread tools/raxmlng/raxmlng.xml Outdated
</param>
<param name="infile" argument="--msa" type="data" format="fasta,phylip" label="Source file with aligned sequences"
help="At least four aligned genomes are needed for RAxML-NG." />
<macros>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you maybe move that into a separate file, that makes it more comprehensible / reviewable I think.

Thanks @amkozlov

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, like this?

Comment thread tools/raxmlng/raxmlng.xml Outdated
--model '$model.auto_seq_type'
#elif $model.model_type == 'single_gui':
#if $model.seq_type == 'multistate':
#set model_matrix = '""'.join(["MULTI", str($model.single_model.multi_state_count),"_",str($model.single_model.subst_matrix)])
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have difficulty to understand what '""'.join(...) is doing..

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for multi-state models, model name contains the number of states, eg. MULTI8_GTR for 8 states.

the might be a more elegant way to build this string, but IIRC + didn't work so I used join() instead

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But this will produce a string with lots of additional " characters in it (two quote characters in between each array element will be added), e.g. '""'.join(["MULTI", "8", "_", "COUNT"]) = 'MULTI""8""_""COUNT'.

What you probably want is ''.join(["MULTI", str($model.single_model.multi_state_count),"_",str($model.single_model.subst_matrix)]).

But also "MULTI" + str($model.single_model.multi_state_count) + "_" + str($model.single_model.subst_matrix) should work.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok I changed it as you suggested and it works - thanks for the hint!

Copy link
Copy Markdown
Contributor

@bernt-matthias bernt-matthias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could definitely need a better test coverage for this tool? In particular for such a larger restructuring?

Comment thread tools/raxmlng/raxmlng.xml Outdated
</xrefs>
<requirements>
<requirement type="package" version="@TOOL_VERSION@">raxml-ng</requirement>
<requirement type="package" version="@TOOL_VERSION@">raxml-ng</requirement>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please restore the old correct indentation.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Comment thread tools/raxmlng/raxmlng.xml
</data>
<data format="nhx" name="startTree" from_work_dir="galaxy.raxml.startTree" label="${tool.name} on ${on_string}: Starting trees">
<filter>general_opts['command'] in ["--search", "--search1", "--all", "--start"] </filter>
<expand macro="m_raxmlng_nofiles_filter"/>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the filetype token needs to be used here, or? Maybe also in other outputs?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I will rather remove this token and move the respective logic out of the macros,

I'm trying to do some (pretty convoluted) output filtering, consider this example:

 <data format="nhx" name="startTree" from_work_dir="galaxy.raxml.startTree" label="${tool.name} on ${on_string}: Starting trees">

<filter>
general_opts['cmdtype']['command'] == "--start" or 
(general_opts['cmdtype']['command'] in ["--search", "--fast", "--all", "--allfast"] and output_opts['keep_interim_files'] and "startTree" in output_opts['keep_interim_files']) 
</filter>

</data>

Intended behavior:

  • startTree file is primary result for one command (--start), but rarely used intermediate results for others
  • so for those other commands, startTree file will be ignored by default - unless user sets a respective checkbox (in output_opts['keep_interim_files']) to indicate that they do want to see this file in the output

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw I noticed a weird thing:

if no checkboxes are set and output_opts['keep_interim_files'] is empty (undefined?), then "startTree" in output_opts['keep_interim_files']evaluates to true.

is this intended behavior or was I hallucinating like with join()? :)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"startTree" in output_opts['keep_interim_files'] will trigger an Exception (TypeError) if output_opts['keep_interim_files'] unchecked (then it is None).

If a filter triggers an exception the file will be included. Therefore you need to check for

output_opts['keep_interim_files'] and "startTree" in output_opts['keep_interim_files']

Comment thread tools/raxmlng/raxmlng.xml
<expand macro="m_raxmlng_nofiles_filter"/>
<filter>general_opts['cmdtype']['command'] in ["--all", "--support", "--bsref"] and "fbp" in general_opts['cmdtype']['bs_metric']</filter>
</data>
<data format="nhx" name="supportTreeTBE" from_work_dir="galaxy.raxml.supportTBE" label="${tool.name} on ${on_string}: ML Tree with branch support values (TBE)">
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please cover new outputs in tests?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, will do.
I have a related question:

I have some output files which are optional and depend on the input file content (not input parameters). E.g. a raxml.collapsedBestTree is only created when final tree has short branches that has been collapsed.
How do I define such outputs in XML? Basically, I want the following:

  • if the file exists, it should appear in history
  • if the file doesn't exist, it's not an error but there should be no respective history entry

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is not possible. If its 1 or more files this might be an option: https://planemo.readthedocs.io/en/latest/writing_advanced.html#examples

Alternatively but such files in a collection, i.e. all such optional outputs in one collection. Then users can extract them from the collection by the name.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok thanks, I'll look into it!

@amkozlov
Copy link
Copy Markdown
Contributor Author

We could definitely need a better test coverage for this tool? In particular for such a larger restructuring?

I added some more tests, please have a look.

<token name="@TOOL_VERSION@">2.0.0</token>
<token name="@VERSION_SUFFIX@">0</token>
<xml name="m_raxmlng_msa">
<param name="infile" argument="--msa" type="data" format="fasta,phylip" label="Input file with aligned sequences (FASTA/PHYLIP)"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could add a validator here and ask for fasta and phylip files with sequences >= 4

https://github.com/galaxyproject/galaxy/blob/4220f5b4240014fd4a6aa8e69bace06551f23ff6/lib/galaxy/datatypes/phylip.py#L38

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done, thanks for the hint!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants