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

Wigbigwig #5704

Merged
merged 163 commits into from
Jan 18, 2024
Merged

Wigbigwig #5704

merged 163 commits into from
Jan 18, 2024

Conversation

fubar2
Copy link
Collaborator

@fubar2 fubar2 commented Jan 10, 2024

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

What:

  • Allow history reference genomes to be used to make bedgraph files and thus bigwig tracks.
  • Update ucsc-wigtobigwig to release 447 - no longer allows stdin so needs the grep output as an input file.
  • Add profile and a citation so it passes planemo lint

Why:

  • The existing converter can only be used with a built-in reference genome.
  • VGP workflows typically use history reference genomes and bigwig is handy for JBrowse2 to speed up loading times for >10M bed feature tracks.

Moving this to the IUC was suggested by @mvdbeek in galaxyproject/galaxy#17261 (comment)

We shouldn't update build-in tools in such major ways. Can you export this to the IUC if there's no other tool doing this ?

Ross agrees - the codebase is designed to be flavoured with plugin tools. Moving text/genome built in tools to IUC suites makes a lot of sense to me in the long term since not every server wants or needs them. They will all possibly want the upload tool.
Simpler and modular is going to help make the codebase more sustainable and the IUC seems a natural way to deal with these tools as they need fixing going forward. Not arguing for rip and replace - not broken enough and no resources...

fubar2 and others added 30 commits December 24, 2023 13:15
blastxml doesn't crash - hard to test..
…nditionals.

Such fun was had discovering this.
@fubar2
Copy link
Collaborator Author

fubar2 commented Jan 11, 2024

@bgruening no, I had accidentally edited the moab/tool_data_table_conf.xml.test so planemo was mixing things up. If you look higher up you'll see planemo linting moab. Seems fine now but it was an interesting experience to track it down...

No idea. Maybe another rebase?

@fubar2 fubar2 requested a review from bgruening January 11, 2024 03:49
Copy link
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.

@@ -0,0 +1,133 @@
<tool id="wigtobigwig" name="Wig-BedGraph-to-bigWig" version="1.1.2" profile="22.05">
Copy link
Contributor

Choose a reason for hiding this comment

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

Please us a version macro and used here and in the requirement version.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you open a PR removing the tool from the Galaxy code base / at least from the tool conf https://github.com/galaxyproject/galaxy/blob/fd45e1bd9fc1f351bb2bb203659758b13173d94c/lib/galaxy/config/sample/tool_conf.xml.sample#L78

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can prefix the tool name by UCSC?

Copy link
Member

Choose a reason for hiding this comment

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

Can you open a PR removing the tool from the Galaxy code base / at least from the tool conf https://github.com/galaxyproject/galaxy/blob/fd45e1bd9fc1f351bb2bb203659758b13173d94c/lib/galaxy/config/sample/tool_conf.xml.sample#L78

No, this needs to stay for reproducibility.

Copy link
Member

Choose a reason for hiding this comment

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

You can hide it though (hidden="True" in the tool conf file)

#else:
-clip
#end if
2>&1 || echo "Error running wigToBigWig." >&2
Copy link
Contributor

Choose a reason for hiding this comment

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

Would prefer that stderr is not redirected to stdout.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is what the converter code has been since lasted touched 6 years ago :)
Not my idea but of course.

@@ -0,0 +1,133 @@
<tool id="wigtobigwig" name="Wig-BedGraph-to-bigWig" version="1.1.2" profile="22.05">
<description>bedGraph or Wig to bigWig converter</description>
Copy link
Contributor

Choose a reason for hiding this comment

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

Just make this the tool name and remove the descrpition.

</when>
</conditional>
<conditional name="settings">
<param name="settingsType" type="select" label="Converter settings to use" help="Default settings should usually be used.">
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you make the indentation consistent?

<conditional name="hist_or_builtin">
<param name="genome_type_select" type="select" label="Reference genome source"
help="Was the Wig/bedgraph input constructed using a genome from your history, or a built-in genome?">
<option selected="True" value="indexed">Input data is for a built-in genome</option>
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a test for the indexed case?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I cannot.
I have tried. Cannot get anyting to work in CI even if it passes biocontainer and ordinary tests - so someone will need to help out for that to work. Please fix if you know how. It's an odd situation because the genome is being read as metadata from the input, Not sure how to do that in tests so advice appreciated..

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add the test case that should run .. then we can have a look on the error.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

will do tomorrow. Late here.

Copy link
Member

Choose a reason for hiding this comment

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

Turns out to not be easy. chromInfo seems to be a special case in Galaxy. I added a test but disabled it.

@fubar2
Copy link
Collaborator Author

fubar2 commented Jan 11, 2024

Would be cool to have this included here: https://github.com/galaxyproject/tools-iuc/tree/main/tools/ucsc_tools

@bernt-matthias - it's up to the iuc to decide where it goes. I don't mind at all. How does the iuc decide where it wants this?

@bernt-matthias
Copy link
Contributor

How does the iuc decide where it wants this?

There is no standard. I just like to have tools coming from the same source close together.

@bgruening
Copy link
Member

@fubar2 I can take this over if you like. Thanks for your work!

@fubar2
Copy link
Collaborator Author

fubar2 commented Jan 14, 2024

I can take this over if you like.
@bgruening - that would be wonderful - yes please.

@fubar2 fubar2 requested a review from nsoranzo as a code owner January 14, 2024 10:49
@bgruening
Copy link
Member

Ok, I moved the wrapper and fixed the review comments.

Copy link
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.

chromInfo also works (tested locally) .. but if I'm not wrong __dbkey__ is the better and testable way to go.

@bgruening bgruening merged commit ab36a3c into galaxyproject:main Jan 18, 2024
11 checks passed
bernt-matthias added a commit to bernt-matthias/galaxy that referenced this pull request Jan 18, 2024
bernt-matthias added a commit to bernt-matthias/galaxy that referenced this pull request Feb 15, 2024
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.

None yet

5 participants