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

tag_pileup_frequency test fixes #2727

Merged
merged 8 commits into from
Dec 5, 2019

Conversation

gregvonkuster
Copy link
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)

@@ -64,12 +64,8 @@ java -jar '$__tool_directory__/TagPileup.jar'
<param name="proper_pe" value="false" />
<param name="combine_strands" value="0" />
<param name="composite_smoothing_window" value="0" />
<param name="output_heatmap" value="true" />
<param name="output_heatmap" value="false" />
Copy link
Member

@mvdbeek mvdbeek Dec 4, 2019

Choose a reason for hiding this comment

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

Should I give this a go ? I assume this test used to work prior to isolating the upload paths.
I suspect this will always generate not very user-friendly names in the collections (dataset_<dataset_id>)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, please give it a go - I'd love to see how this should work! ;) Thanks for the help.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The output naming convention for heatmap ouitput collection elements is something like __read1_anti.tabular

Copy link
Contributor

Choose a reason for hiding this comment

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

We should add a test that also outputs the heatmaps in order to see if the updated requirements work.

…ally generated output file names in tool config test name attribute - test coverage still good."

This reverts commit 6040fcd.
@@ -64,12 +64,8 @@ java -jar '$__tool_directory__/TagPileup.jar'
<param name="proper_pe" value="false" />
<param name="combine_strands" value="0" />
<param name="composite_smoothing_window" value="0" />
<param name="output_heatmap" value="true" />
<param name="output_heatmap" value="false" />
Copy link
Contributor

Choose a reason for hiding this comment

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

We should add a test that also outputs the heatmaps in order to see if the updated requirements work.

@mvdbeek
Copy link
Member

mvdbeek commented Dec 5, 2019

@bernt-matthias I've restored the test, but it does look like this tool doesn't do any plotting (only tabullar outputs). I'll see if it passes without the fonts as well.

@mvdbeek
Copy link
Member

mvdbeek commented Dec 5, 2019

Yep, works fine without the fonts.

mvdbeek added a commit to mvdbeek/tools-iuc that referenced this pull request Dec 5, 2019
@mvdbeek mvdbeek merged commit 7ea7f1c into galaxyproject:master Dec 5, 2019
@gregvonkuster gregvonkuster deleted the tag_pileup_freq branch December 10, 2019 13:20
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