Skip to content

Conversation

lsterck
Copy link
Contributor

@lsterck lsterck commented Sep 25, 2025

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)

This pull request adds the RIPPLES tool to the usher suit.
Since usher version was a 0.2.1 and RIPPLES only added in v 0.4.0, the whole package was updated to the latest version (0.6.6)
Extra options were added that were released in the past 4-5y , but perhaps not all of them

linked to issue #7242

@lsterck lsterck marked this pull request as ready for review September 28, 2025 20:20
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.

Cool, a few comments inline.

matUtils
$matutils_mode.options_mode
--input-mat $mutation_annotation_file
--input-mat '$mutation_annotation_file.name'
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
--input-mat '$mutation_annotation_file.name'
--input-mat '$mutation_annotation_file.element_identifier'

@@ -7,9 +7,12 @@
<expand macro='requirements' />
<version_command>usher --version</version_command>
<command detect_errors='exit_code'><![CDATA[
## get correct extension filenames
ln -sf '$mutation_annotation_file' '$mutation_annotation_file.name' &&
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ln -sf '$mutation_annotation_file' '$mutation_annotation_file.name' &&
ln -sf '$mutation_annotation_file' '$mutation_annotation_file.element_identifier' &&

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll look into it.
I think it specifically needs the correct extension ...

@@ -195,7 +216,7 @@
<option value="write-mat">Write the selected subtree as a mutation annotated tree (--write-math)</option>
<option value="write-mat-collapsed">Write the selected subtree as a collapsed mutation annotated tree (--write-mat --collapsed)</option>
<option value="write-json">Write an Auspice-compatbile json representing the selected subtree (--write-json)</option>
<option value="write-tree"> Write a newick string representing the selected subtree (--write-tree)</option>
<option value="write-tree"> Write a newick strineg representing the selected subtree (--write-tree)</option>
Copy link
Member

Choose a reason for hiding this comment

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

that looks wrong now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤦‍♂️ it sure does 🙂

@lsterck lsterck requested a review from bgruening October 1, 2025 07:08
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.

Upps forget to submit my review. Thanks @lsterck

@@ -1,4 +1,4 @@
<tool id='usher_matutils' name='UShER matUtils' version='@TOOL_VERSION@+@GALAXY_TOOL_VERSION@' profile='23.2'>
<tool id='usher_matutils' name='UShER matUtils' version='@TOOL_VERSION@+@GALAXY_TOOL_VERSION@' profile='20.02'>
Copy link
Member

Choose a reason for hiding this comment

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

Why such an old profile?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

dunno
that was what was there before.
I thought this indicate the min version that was needed to run the software?

<output name="parent_placements" file="test_22_parent_placements.tabular" ftype="tabular"/>
<output name="parent_placements" ftype="tabular">
<assert_contents>
<has_size value="199899" delta="300"/>
Copy link
Member

Choose a reason for hiding this comment

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

size comparisons are not good tests. They are not very stingent, can you assert some content? Or assert the line count, or column count?

Copy link
Contributor Author

@lsterck lsterck Oct 9, 2025

Choose a reason for hiding this comment

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

Good thing you pointed me to change the test checks (though those tests were already there)
In the process of changing it to file checks it turned out there was a nasty bug hidden in one of the parameter processing causing that specific test to always fail (could technical not even be run) but since check on output size it passed the wrapper test.

]]> </command>
<inputs>
<param argument="--input-mat" type="data" format="protobuf3" label="Mutation-annotated tree object" help="Load a mutation annotated tree file, in protocol-buffers format (protobuf3)."/>
Copy link
Member

Choose a reason for hiding this comment

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

please consider adding min/max to all integers/float params

Copy link
Contributor Author

@lsterck lsterck Oct 7, 2025

Choose a reason for hiding this comment

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

Would like to, but have no idea what sensible ranges would be ...
As I understand it is very dependent from the input/output trees used and as such it does not really make sense to "fix" ranges

<param argument="--num-descendants" type="integer" value="10" label="Number of descendants" help="Minimum number of leaves that node should have to be considered for recombinatino. Default = 10." />
</inputs>
<outputs>
<data name="recombination" format="tabular" from_work_dir='recombination.tsv' label="${tool.name} on ${on_string}: recombinations" />
Copy link
Member

Choose a reason for hiding this comment

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

do you want to add metadata for the column names here?

@lsterck
Copy link
Contributor Author

lsterck commented Oct 10, 2025

one test failed with :

History not found

what does that mean actually? @bgruening

anyway, maybe I should remove even more of those file_size checks

@SaimMomin12
Copy link
Contributor

one test failed with :

History not found

what does that mean actually? @bgruening

anyway, maybe I should remove even more of those file_size checks

Let me restart the CI

@lsterck lsterck requested a review from bgruening October 13, 2025 14:13
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