kylebgorman / pynini

Read-only mirror of Pynini
http://pynini.opengrm.org
Apache License 2.0
118 stars 27 forks source link

dot default SVG rendering broken #35

Closed lxkain closed 2 years ago

lxkain commented 3 years ago

As of today, using the latest pynini and openFST sources, pip installs, and a base ubuntu 20.04, the default SVG rendering of graphviz's dot is broken. This is of course not pynini's fault, but it affects its output in jupyter notebooks. After much searching, I came upon a workaround that others might find useful:

import graphviz
from IPython.display import SVG, display
def draw(fst):
    fst.draw('tmp.dot', portrait=True, isymbols=fst.input_symbols(), osymbols=fst.output_symbols())
    graphviz.render('dot', 'svg', 'tmp.dot', renderer='cairo')
    return display(SVG(url='file:tmp.dot.cairo.svg'))

The key here is to use svg:cairo rendering. You can then use draw(fst). I wrote some code that tried to overwrite a fst object's _reprsvg function, but it's read-only, so for now I am using the functional version.

kylebgorman commented 3 years ago

Thanks Alex. Do you know what's broken?

I generally like the idea of moving the rendering from a command-line shell-out to using the built-in graphviz, and maybe I ought to do that anyways in the next release.

On Tue, Jan 5, 2021 at 3:55 PM Alexander Kain notifications@github.com wrote:

As of today, using the latest pynini and openFST sources, pip installs, and a base ubuntu 20.04, the default SVG rendering of graphviz's dot is broken. This is of course not pynini's fault, but it affects its output in jupyter notebooks. After much searching, I came upon a workaround that others might find useful:

import graphviz from IPython.display import SVG, display def draw(fst): fst.draw('tmp.dot', portrait=True, isymbols=fst.input_symbols(), osymbols=fst.output_symbols()) graphviz.render('dot', 'svg', 'tmp.dot', renderer='cairo') return display(SVG(url='file:tmp.dot.cairo.svg'))

The key here is to use svg:cairo rendering. You can then use draw(fst). I wrote some code that tried to overwrite a fst object's repr_svg function, but it's read-only, so for now I am using the functional version.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/kylebgorman/pynini/issues/35, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABG4ONFWX6HK26KJWZYGEDSYN4FZANCNFSM4VWKFXPQ .

lxkain commented 3 years ago

If you remove the keyword argument renderer it will use the default SVG render (i.e. dot -Tsvg) which for some reason (I have not looked below this level) makes mistakes with the bounding box, resulting in wrongly sized and cropped outputs in notebooks.

If it weren't for this, I could just use graphviz.Source.from_file, but this fails (and it doesn't have the render option), as does the built-in approach.

kylebgorman commented 3 years ago

Actually, I don't think I want to use this approach in general because I just read the source code for the graphviz PyPi package and it just uses subprocess also, so there's no big improvement here. It seems like we just need to add the Cairo renderer to the command used.

On Tue, Jan 5, 2021 at 5:38 PM Alexander Kain notifications@github.com wrote:

Closed #35 https://github.com/kylebgorman/pynini/issues/35.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/kylebgorman/pynini/issues/35#event-4172339586, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABG4ON2VYN764UEVVZOKRDSYOIHXANCNFSM4VWKFXPQ .

lxkain commented 3 years ago

I think that would do the trick until this is fixed in graphviz itself, agreed. Thank you again for continuing development on this package!

lxkain commented 3 years ago

Any developments on this front?

kylebgorman commented 3 years ago

Alright, so where we were is that we have nothing to gain from using the graphviz package but that we want it to use the Cairo renderer, is that right? One reason I didn't make any progress on this is I can't quickly replicate the issue, though if you're willing to try it out locally, that'd be sufficient for me to make the change for the next release.

So I think the format command should be -Tcairo:svg, is that your impression? That ought to mean that the renderer is Cairo and the format is SVG. If that's correct, you could take the current release, modify this line to reflect, then recompile and test locally. Would you be willing to do that at some point?

lxkain commented 3 years ago

Absolutely, will do this shortly.

kylebgorman commented 3 years ago

Thanks so much. If you can confirm it works for you, I'll test to make sure it doesn't make things worse for me locally, then I can push the change out.

On Fri, Apr 23, 2021 at 11:28 AM Alexander Kain @.***> wrote:

Absolutely, will do this shortly.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/kylebgorman/pynini/issues/35#issuecomment-825736601, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABG4OMN4HD6HPO6JYKVFVDTKGGZZANCNFSM4VWKFXPQ .

lxkain commented 3 years ago

-Tsvg:cairo appears to have fixed the issue for me.

kylebgorman commented 3 years ago

Alright, I'll try myself and if it works it'll be in the next release. Won't close this until I determine.

On Fri, Apr 23, 2021 at 8:11 PM Alexander Kain @.***> wrote:

-Tsvg:cairo appears to have fixed the issue for me.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/kylebgorman/pynini/issues/35#issuecomment-826000362, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABG4OIYSASVZEEWB2LS3J3TKIEBTANCNFSM4VWKFXPQ .

kylebgorman commented 3 years ago

Have determined this works, so I'll put it in the next release.

On Fri, Apr 23, 2021 at 8:11 PM Kyle Gorman @.***> wrote:

Alright, I'll try myself and if it works it'll be in the next release. Won't close this until I determine.

On Fri, Apr 23, 2021 at 8:11 PM Alexander Kain @.***> wrote:

-Tsvg:cairo appears to have fixed the issue for me.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/kylebgorman/pynini/issues/35#issuecomment-826000362, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABG4OIYSASVZEEWB2LS3J3TKIEBTANCNFSM4VWKFXPQ .