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.

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

added squirrel MPXV analysis #6666

Merged
merged 30 commits into from
Jan 16, 2025
Merged

Conversation

ammaraziz
Copy link
Contributor

First time contributing a galaxy wrapper. Please let me know if there any issues I need to fix.

One possible issue that might cause a block to merging is the lint warning I get:

.. WARNING (HelpInvalidRST): Invalid reStructuredText found in help - [<string>:5: (ERROR/3) Unexpected indentation.
].

Unsure how to solve this issue. Any guidance would be appreciated.

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)

@ammaraziz
Copy link
Contributor Author

Looking at the errors, in order:

  1. Test file size is ~5mb. How do I deal with this issue when the mpxv genomes are large? Testing the alignment
  2. Same as above, some of the tests use big files.
  3. Tests failed, I think I can identify the issue.
  4. Linting fails due to the above mentioned error. Any advice?

Thanks!

@ammaraziz
Copy link
Contributor Author

Regarding the tests, they are erroring due to differences between what is uploaded and what is run. The outputs contain some date information (I am technically in the future compared to the github runner time zone). If I wait until tomorrow it will probably pass...

Any advice on how to manage such situations?

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.

Very cool, thanks a lot.

My initial first columns are inline. For the file size, can you just use a subset of the input size? Can you compress it?

tools/squirrel/squirrel-phylo.xml Outdated Show resolved Hide resolved
tools/squirrel/squirrel-phylo.xml Outdated Show resolved Hide resolved
tools/squirrel/squirrel-phylo.xml Outdated Show resolved Hide resolved
tools/squirrel/squirrel-phylo.xml Outdated Show resolved Hide resolved
tools/squirrel/squirrel-phylo.xml Outdated Show resolved Hide resolved
tools/squirrel/squirrel-phylo.xml Outdated Show resolved Hide resolved
tools/squirrel/squirrel-phylo.xml Outdated Show resolved Hide resolved
tools/squirrel/squirrel-qc.xml Outdated Show resolved Hide resolved
tools/squirrel/squirrel-qc.xml Outdated Show resolved Hide resolved
tools/squirrel/squirrel-qc.xml Outdated Show resolved Hide resolved
@ammaraziz
Copy link
Contributor Author

My initial first columns are inline. For the file size, can you just use a subset of the input size? Can you compress it?

Unfortunately not but I think I will recreate the background within the input file for testing to replicate the --include-background flag.

Thanks for all your assistance!

@bgruening
Copy link
Member

Keep in mind that the Galaxy tests are for testing the Galaxy wrapper, not the functionality of the tool. So small test data even if the result is not super useful is valid. We also can check for tests that we know are failing etc ...

@bgruening
Copy link
Member

I see this in the tests: > Incorrect number of outputs - expected 2, found 3 (dataset(s): html,mask,exclude collection(s): )

@ammaraziz
Copy link
Contributor Author

🥳 all tests pass. Thanks everyone for your help and especially your guidance @bgruening. There's still the issue of how I'm parsing the inputs, which I will fix now.

@ammaraziz
Copy link
Contributor Author

Ready for (proper) review. All the issues should be fixed now.

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, just the few open comments :)

type="data"
format="fasta"
label="Sequences in fasta format"
help="You can upload a FASTA sequence to the history and use it as reference" />
Copy link
Member

Choose a reason for hiding this comment

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

ping ...

tools/squirrel/squirrel-phylo.xml Outdated Show resolved Hide resolved
tools/squirrel/squirrel-qc.xml Outdated Show resolved Hide resolved
@@ -0,0 +1,211 @@
<tool id="squirrel_phylo" name="squirrel phylo" version="@TOOL_VERSION@+galaxy@VERSION_SUFFIX@" profile="21.05">
<description>phylogenetic analysis of MPXV</description>
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps use some more capitalization and descriptive text so that this shows up nicely in the tool list? E.g. "MPXV (Mpox virus)" and "Squirrel phylogenetic analysis" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a really good suggestion. I noticed the name and description fields are concatenated in this manner: {Name} - {Description}. It seems to me the searching happens on both? Or do you recommend the description be as descriptive as possible even if it contains duplicate information?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pvanheus I've updated the description and name. Let me know if it suits.

@ammaraziz ammaraziz requested a review from pvanheus January 16, 2025 01:58
@@ -0,0 +1,211 @@
<tool id="squirrel_phylo" name="Squirrel Phylo" version="@TOOL_VERSION@+galaxy@VERSION_SUFFIX@" profile="21.05">
<descriptionp>Phylogenetic and APOBEC3 analysis of MPXV (Mpox virus)</description>
Copy link
Contributor

Choose a reason for hiding this comment

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

The typo here (descriptionp) is causing the linting problem that causes tests to fail (i.e. the linting problem seems to refer to the .shed.yml but it is caused by the failure to parse the squirrel-phylo.xml file)

@pvanheus pvanheus merged commit ed19e40 into galaxyproject:main Jan 16, 2025
12 checks passed
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.

4 participants