Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add Quicktree tool #6583

Merged
merged 12 commits into from
Nov 25, 2024
Merged

Add Quicktree tool #6583

merged 12 commits into from
Nov 25, 2024

Conversation

shiltemann
Copy link
Member

Add Quicktree tool wrapper

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)

bgruening
bgruening approved these changes Nov 22, 2024
View reviewed changes
Copy link
Member

@bgruening bgruening left a comment

Choose a reason for hiding this comment

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

Great! Just one comment, not sure if useful. Feel free to merge if not.

tools/quicktree/quicktree.xml
<command detect_errors="exit_code"><![CDATA[

## convert alignment to stockholm before quicktree if needed
#if $input_type.format == "align"
Copy link
Member

Choose a reason for hiding this comment

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

You could also do this if/else dance based on the filetype $input_type.ext and avoid the conditional. But maybe explicit is better than implicit here.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, I did it this way because "phylip" format can unhelpfully refer to either a distance matrix format or an alignment format in this tool so I figured I would make it explicit for the user which they are supplying. But I will check if Galaxy has a sniffer for phylip and which format it assumes when a file is marked as phylip and see if I can streamline that part.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, looks like Galaxy's definition of phylip is the alignment version, but that still leaves the issue that distance matrices and some of the supported alignment file types will both be 'txt' in Galaxy currently so I think the conditional is still good here

Copy link
Member

Choose a reason for hiding this comment

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

Thanks. Ship it if your like I will make sure its installed on Monday.

Copy link
Contributor

Choose a reason for hiding this comment

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

Independent of this PR, esl-reformat might be cool as extra tool.

bgruening
bgruening reviewed Nov 22, 2024
View reviewed changes
tools/quicktree/quicktree.xml Outdated
<param name="input_file" type="data" format="fasta,stockholm,phylip,txt" label="Alignment file" />
</when>
<when value="dist">
<param name="input_file" type="data" format="mothur.dist,txt" label="Distance Matrix" />
Copy link
Member

Choose a reason for hiding this comment

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

maybe describe a bit this distance matrix format ... I'm super lost with those formats :(

Copy link
Member Author

Choose a reason for hiding this comment

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

will do 👍

bernt-matthias
bernt-matthias approved these changes Nov 22, 2024
View reviewed changes
tools/quicktree/quicktree.xml Outdated
<outputs>
<data name="output_file" format="newick" label="${tool.name} on ${on_string}: stockholm format">
<change_format>
<when input="output_type" value="dist_out" format="dist" />
Copy link
Contributor

Choose a reason for hiding this comment

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

What is dist?

Copy link
Member Author

Choose a reason for hiding this comment

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

good catch, I thought there was a dist format but I guess only mothur.dist exists currently. mothur.dist is the correct format here, but the mothur prefix might be confusing

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, we currently have mothur.dist and if you want to distinguish between a square matrix or a lower triangle matrix there are mothur.lower.dist or mothur.square.dist. But mothur uses the phylip format, so maybe it would be good to make aliases in Galaxy for these formats (e.g. phylip.dist, phylip.dist.square etc)

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds like a very good good idea.

tools/quicktree/quicktree.xml
<command detect_errors="exit_code"><![CDATA[

## convert alignment to stockholm before quicktree if needed
#if $input_type.format == "align"
Copy link
Contributor

Choose a reason for hiding this comment

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

Independent of this PR, esl-reformat might be cool as extra tool.

bernt-matthias
bernt-matthias reviewed Nov 25, 2024
View reviewed changes
tools/quicktree/quicktree.xml Outdated
@@ -56,7 +56,7 @@ input.quicktree
<param name="input_file" type="data" format="fasta,stockholm,phylip,txt" label="Alignment file" />
</when>
<when value="dist">
<param name="input_file" type="data" format="mothur.dist,txt" label="Distance Matrix" />
<param name="input_file" type="data" format="mothur.dist,mothur.lower.dist,mothur.square.dixt,txt" label="Distance Matrix" help="A distance matrix in phylip format (see help below for details). Can be a square distance matrix or a lower triangle distance matrix." />
Copy link
Contributor

Choose a reason for hiding this comment

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

  • mothur.square.dixt -> mothur.square.dist
  • txt can / should we replace this by tabular? Or at least add it?

Copy link
Contributor

Choose a reason for hiding this comment

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

should we replace this by tabular

I answer this myself: No :)

tools/quicktree/quicktree.xml Outdated
<outputs>
<data name="output_file" format="newick" label="${tool.name} on ${on_string}: stockholm format">
<change_format>
<when input="output_type" value="dist_out" format="dist" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds like a very good good idea.

@shiltemann shiltemann merged commit ba1e44d into galaxyproject:main Nov 25, 2024
11 checks passed
@shiltemann
Copy link
Member Author

thanks @bernt-matthias and @bgruening for the review 👍

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

@bernt-matthias bernt-matthias bernt-matthias approved these changes

@bgruening bgruening bgruening approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@shiltemann @bgruening @bernt-matthias