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

newick display: add imagemagick dependency #2710

Merged

Conversation

bernt-matthias
Copy link
Contributor

imagemagick requirement was missing anyway. since for some reason
neither imagemagick not graphicsmagick can convert the image I
suggest to stick to svg only for now.

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)

imagemagick requirement was missing anyway. since for some reason
neither imagemagick not graphicsmagick can convert the image I
suggest to stick to svg only for now.
@@ -36,15 +36,7 @@ nw_display
-w $width
$radial

'$fileNewick' > output.svg

#if $outformat == 'png':
Copy link
Member

Choose a reason for hiding this comment

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

Is that maybe also due to the bug fixed by galaxyproject/galaxy#9052 ? Because this would just move output.svg '$output' instead of running the conversion ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nope. convert was just not installed. The container test output was convert: command not found.

I tried on my local system to use convert and gm convert and both were not able to create a png from the svg.

Copy link
Member

Choose a reason for hiding this comment

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

I was able to add imagemagick from conda-forge and got a png with this patch:

diff --git a/tools/newick_utils/newick_display.xml b/tools/newick_utils/newick_display.xml
index fbe029b5e..5dcc7b17a 100644
--- a/tools/newick_utils/newick_display.xml
+++ b/tools/newick_utils/newick_display.xml
@@ -5,6 +5,7 @@
     </edam_operations>
     <requirements>
         <requirement type="package" version="1.6">newick_utils</requirement>
+        <requirement type="package" version="7.0.9_6">imagemagick</requirement>
     </requirements>
     <command detect_errors="aggressive"><![CDATA[
 nw_display

But the png seems to miss fonts ?
tree

Copy link
Member

@mvdbeek mvdbeek Nov 30, 2019

Choose a reason for hiding this comment

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

Cool, we can add a minimal fonts package like this to fix it:

diff --git a/tools/newick_utils/newick_display.xml b/tools/newick_utils/newick_display.xml
index fbe029b5e..c75f8685c 100644
--- a/tools/newick_utils/newick_display.xml
+++ b/tools/newick_utils/newick_display.xml
@@ -5,6 +5,8 @@
     </edam_operations>
     <requirements>
         <requirement type="package" version="1.6">newick_utils</requirement>
+        <requirement type="package" version="7.0.9_6">imagemagick</requirement>
+        <requirement type="package" version="1">fonts-conda-forge</requirement>
     </requirements>
     <command detect_errors="aggressive"><![CDATA[
 nw_display
@@ -110,7 +112,7 @@ nw_display
             <param name="radial" value="-r"/>
             <param name="branchlength" value="true"/>
             <param name="outformat" value="png"/>
-            <output name="output" file="tree.png" ftype="png" compare="sim_size" delta="15000"/>
+            <output name="output" file="tree.png" ftype="png" compare="sim_size" delta="18000"/>
         </test>
          <test><!-- test with txt output format -->
             <param name="fileNewick" value="tree.nwk"/>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

great. woudn't it be better to add the fonts package as a requirement to the imagemagick recipe?

Copy link
Member

Choose a reason for hiding this comment

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

Possibly, I'm not sure how that decision is made. It seems imagemagick will use whatever is available.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lets see conda-forge/imagemagick-feedstock#101 .. I will restore the original code.

Copy link
Member

Choose a reason for hiding this comment

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

out of interest ... does graphicsmagick work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

gm gives the following error gm convert: invalid primitive argument (small).

bernt-matthias added a commit to bernt-matthias/imagemagick-feedstock that referenced this pull request Dec 1, 2019
For the conversion from svg to png fonts are needed. See discussion
here: galaxyproject/tools-iuc#2710
restored png output

slightly better redirection
Copy link
Member

@mvdbeek mvdbeek left a comment

Choose a reason for hiding this comment

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

Cool!

@mvdbeek mvdbeek changed the title newick display: remove png output newick display: add imagemagick dependency Dec 1, 2019
@mvdbeek mvdbeek merged commit 352b2b1 into galaxyproject:master Dec 1, 2019
@bernt-matthias bernt-matthias deleted the topic/nw-container-fixes branch March 8, 2020 10:38
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