ENH: Add support for capturing svg, png, and jpeg and _repr_mimebundle_ #1138

oscargus · 2023-05-13T10:53:24Z

However, I have a doubt that this is a bit "too hacky" as it now checks for both str and bytes to check that a valid representation is obtained. I guess that there is a reason for the explicit str-check? So adding bytes may break things. I was thinking about adding some support to tell if bytes is OK for a given repr (it is required for png), but didn't really find a good way. It is of course possible to drop support for png and this would go away. (It would also be easy to add support for jpeg.)

Also, these are now directly embedded in the source, leading to that they will not show up as thumbnails. I guess that the same thing happens for objects that only have a _html_repr_ and are not scraped explicitly? Not a big deal for my use case, but would of course be nice even there.

A minor thing, which I sort of realize the reason for, is that these show up at the top. This of course makes sense when it comes to plots, but as I'd like to illustrate the output object in a graphical way, not so much. Easy to solve on my side though. (And I think it only affects the first "cell".)

Should these new representations be on or off by default? I guess having them on may lead to some surprising outputs...

Finally, tests and docs are missing, but I wanted to get some feedback on the bytes-issue before going into that. Edit: I think this is probably a better approach to the previous bytes-issue, but still not sure if embedded is the way to go.

(And, yes, not that many lines were needed, at least not with this approach.)

lucyleeow

Looks good and reasonably simple addition from first glance. Just need test and doc if we're happy to go ahead with adding support for this. Thanks!

sphinx_gallery/gen_rst.py

oscargus · 2023-05-15T08:49:51Z

Thanks! I'll try to add tests and documentation.

oscargus · 2023-05-15T10:54:06Z

This still needs tests for PNG and JPG output, but I cannot really figure out a good way to generate it without adding dependencies... Will try to generate a minimal binary file and play around a bit.

larsoner · 2023-05-15T11:18:55Z

You could make custom classes with the _repr_* methods that wrap to a little Pillow call to dump some reasonable bytes.

oscargus · 2023-05-17T10:01:16Z

I'm now thinking that it may make sense to save the figures to file (one of my use cases is to capture GraphViz graphs which can be quite big, so it is nice to be able to "Open image in new tab" which requires a separate file).

I know that there are thoughts of too many configuration parameters already, but would it make sense to add another one here?
One can possibly merge it with #1134? So that the same parameter determines if all images are embedded (no thumbnails) or saved as separate files (thumbnails)?

It may be that this can be added on later, depending on how hard it is...

larsoner · 2023-05-17T16:32:49Z

I would start out by writing the files to disk just like current scrapers, then adding them as .. img-sg:: or whatever. Let's see if that's good enough. Then if we want to embed these as base64-encoded images or whatever we can do that separately.

oscargus · 2023-05-19T11:44:20Z

OK, so now it writes files. Also, I added support for _repr_mimebundle_ in a way transparent for the user. Some objects, like GraphViz graphs, only support this. But asking for _repr_svg_ will automatically look for an SVG in the MIME bundle (if the method exists etc).

My main problem now is how to test all this. The tests for embedding were quite straightforward as one can simply check the resulting HTML. However, I will need to look more into how to possibly test these ones.

oscargus · 2023-05-19T11:50:51Z

The content of _repr_mimebundle_ takes precedence if it contains the correct type. This makes sense to avoid generating the same content twice. Also:

https://github.com/scikit-learn/scikit-learn/blob/86541f2b3bc8a96264e265cb810cf79858544340/sklearn/base.py#L636-L638

It is hard to say how many more packages can be supported by this. A quick search got ~1200 hits for _repr_html_ compared to ~200 for _repr_svg_ and ~150 for _repr_mimebundle_.

However, as I personally rely on SymPy and GraphViz quite a bit, it is a real game changer for me. :-)

oscargus · 2023-05-19T12:02:56Z

Another issue, I realize from the tests, is that SVGs are not supported by LaTeX out of the box. Of course it is a bit expected, but I wonder if one should add support for converting SVGs to PDFs? Or mention, e.g., https://pypi.org/project/sphinxcontrib-svg2pdfconverter/ in the documentation?

oscargus · 2023-05-19T19:15:17Z

Managed to get thumbnails etc working...

(Edit: well, not me, but calling the right function and voilà.)

larsoner · 2023-05-22T18:15:48Z

Error on CircleCI:

! LaTeX Error: Unknown graphics extension: .svg.

One way to avoid this would be to wrap the SVG output in .. only:: html. But maybe we should use the sphinxcontrib- package you recommend instead? It would be okay to add this to the test requirements file (assuming the module is well maintained)

larsoner · 2023-05-22T18:17:10Z

... but I should add that the code otherwise looks good, and this output shows things nicely:

https://output.circle-artifacts.com/output/job/776959c5-ed38-4544-be3a-1e6a9930c298/artifacts/0/tiny_html/auto_examples/plot_mime_bundle.html

So once we can make CIs happy we should be good

oscargus · 2023-05-23T11:01:20Z

I've tried to make it work, but to no avail. My theory is that the svg2pdf converter is not working since the sg image is an imgsgnode and not an img at that stage. Hence, I tried with using .. image:: rather than .. sg-image::. Then, the thumbnails still works, but no image as the rst-code is kept as is. So while the build nowt passes, it is not correct.

oscargus · 2023-05-23T13:26:04Z

This is the current output and source to the test errors:

https://output.circle-artifacts.com/output/job/ee02670a-3e05-444c-9029-0b0c7b60c191/artifacts/0/tiny_html/auto_examples/plot_mime_bundle.html

As seen, the .. image:: is interpreted as a continuation of the code block. I have (currently) no idea how/where to get around this, so any ideas are appreciated.

larsoner · 2023-05-23T14:30:20Z

I've tried to make it work, but to no avail. My theory is that the svg2pdf converter is not working since the sg image is an imgsgnode and not an img at that stage. Hence, I tried with using .. image:: rather than .. sg-image::. Then, the thumbnails still works, but no image as the rst-code is kept as is. So while the build nowt passes, it is not correct.

For now then let's just detect if it's SVG and wrap it in a .. only:: html block. If it's an issue with img-sg or whatever then it's affecting more than just this PR, so we can/should tackle it separately.

larsoner · 2023-05-23T14:31:12Z

sphinx_gallery/scrapers.py

@@ -353,7 +353,7 @@ def save_figures(block, block_vars, gallery_conf):
    return all_rst


-def figure_rst(figure_list, sources_dir, fig_titles='', srcsetpaths=None):
+def figure_rst(figure_list, sources_dir, fig_titles='', srcsetpaths=None, sg_image=True):


Remove sg_image for now

larsoner · 2023-05-23T14:34:37Z

As seen, the .. image:: is interpreted as a continuation of the code block. I have (currently) no idea how/where to get around this, so any ideas are appreciated.

Locally you could check this, but let's use CircleCI instead. The RST files are actually saved to disk and you can look:

https://output.circle-artifacts.com/output/job/ee02670a-3e05-444c-9029-0b0c7b60c191/artifacts/0/tiny_html/_sources/auto_examples/plot_mime_bundle.rst.txt

You can see the problematic line:

 .. image:: /auto_examples/images/sphx_glr_plot_mime_bundle_001.svg

In particular note the leading space

sphinx_gallery/scrapers.py

oscargus · 2023-06-03T12:11:03Z

I got it fully working using .. sg-image:: for SVG. There is a special template for SVGs without srcset so .. image-sg:: will directly use the parent method here:

sphinx-gallery/sphinx_gallery/directives.py

Lines 186 to 188 in 1e825be

    
           if node['srcset'] is None: 
        
               self.visit_image(node) 
        
               return

Had to install pipwin and from there cairocffi to get the Windows tests passing. But now it seems to work properly:
https://output.circle-artifacts.com/output/job/9efaf816-edc7-46d7-b119-4c6b4f319928/artifacts/0/tiny_html/auto_examples/index.html

Not sure if this is an acceptable solution for now or if it may break something else?

larsoner

Just a few minor comments otherwise LGTM!

larsoner · 2023-06-05T13:32:13Z

sphinx_gallery/scrapers.py

@@ -353,7 +353,8 @@ def save_figures(block, block_vars, gallery_conf):
    return all_rst


-def figure_rst(figure_list, sources_dir, fig_titles='', srcsetpaths=None):
+def figure_rst(figure_list, sources_dir, fig_titles='', srcsetpaths=None,
+               svg_image=False):


Might as well add these because it's easy

Suggested change

svg_image=False):

*, svg_image=False):

larsoner · 2023-06-05T13:32:35Z

sphinx_gallery/gen_rst.py

+    excluded_mime_types = {MIME_MAPPING[repr] for repr in
+                           MIME_MAPPING_REPR.difference(capture_repr_set)}
+    mimebundle = {}
+    if included_mime_types and hasattr(___, '_repr_mimebundle_'):


This hasattr is unnecessary, the try below would catch the AttributeError below anyway. Might as well remove it

Suggested change

if included_mime_types and hasattr(___, '_repr_mimebundle_'):

if included_mime_types:

Yeah, you are correct. I think the idea was to try to catch a slightly more narrow class of exceptions, where AttributeError wasn't one of them, but never managed to figure out what a suitable subset might be.

larsoner · 2023-06-05T13:33:26Z

sphinx_gallery/scrapers.py

+# Used for SVGs
+SVG_IMAGE = """
+.. image:: /%s
+   :alt: %s
+   :class: sphx-glr-single-img
+"""


DRY / this appears to be a duplicate of SINGLE_IMAGE below, it would be better just to use that directly (and fix the leading space in it), or below it write

SVG_IMAGE = SINGLE_IMAGE # alias

Except for that initial space. Since it said it was kept for backwards compatibility I wasn't sure if the space should still be there to not break anything. But maybe the space is always an issue?

Yeah I think the space is a bug and can safely be removed

larsoner

Just a few minor comments otherwise LGTM!

oscargus force-pushed the repr_svg_png branch from 4af7cf8 to 4ee59d9 Compare May 13, 2023 11:12

lucyleeow reviewed May 15, 2023

View reviewed changes

sphinx_gallery/gen_rst.py Outdated Show resolved Hide resolved

sphinx_gallery/gen_rst.py Outdated Show resolved Hide resolved

oscargus changed the title ~~Add support for capturing svg and png~~ ENH: Add support for capturing svg and png May 15, 2023

oscargus force-pushed the repr_svg_png branch 2 times, most recently from c827db0 to e2a1592 Compare May 15, 2023 10:51

oscargus changed the title ~~ENH: Add support for capturing svg and png~~ ENH: Add support for capturing svg, png, and jpeg May 18, 2023

oscargus force-pushed the repr_svg_png branch 7 times, most recently from 2e10238 to 19d569e Compare May 19, 2023 11:40

oscargus force-pushed the repr_svg_png branch from 19d569e to f594782 Compare May 19, 2023 11:48

oscargus changed the title ~~ENH: Add support for capturing svg, png, and jpeg~~ ENH: Add support for capturing svg, png, and jpeg and _repr_mimebundle_ May 19, 2023

oscargus force-pushed the repr_svg_png branch 7 times, most recently from 6c1901e to 243069d Compare May 19, 2023 14:26

oscargus force-pushed the repr_svg_png branch from c73f452 to 800bf74 Compare May 19, 2023 22:46

This was referenced May 22, 2023

Better support for SVGs when building LaTeX #1144

Open

Adapt for sphinx-gallery: add _repr_html_ xflr6/graphviz#199

Closed

xflr6 mentioned this pull request May 22, 2023

Add _repr_html_ method xflr6/graphviz#200

Closed

oscargus force-pushed the repr_svg_png branch 3 times, most recently from 246a7a1 to 1d282e5 Compare May 23, 2023 10:59

oscargus force-pushed the repr_svg_png branch 4 times, most recently from b04e172 to 989934d Compare May 23, 2023 11:47

larsoner reviewed May 23, 2023

View reviewed changes

sphinx_gallery/scrapers.py Show resolved Hide resolved

oscargus force-pushed the repr_svg_png branch 3 times, most recently from 9fe1bd4 to d044ca4 Compare June 3, 2023 11:11

Add support for capturing svg, png, and jpeg and _repr_mimebundle_

370bc6b

oscargus force-pushed the repr_svg_png branch from d044ca4 to 370bc6b Compare June 3, 2023 11:54

oscargus force-pushed the repr_svg_png branch from 70c3249 to 370bc6b Compare June 3, 2023 12:26

larsoner reviewed Jun 5, 2023

View reviewed changes

MarcSkovMadsen mentioned this pull request Sep 16, 2023

Make it easy to use Panel with Sphinx-Gallery holoviz/panel#5519

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: Add support for capturing svg, png, and jpeg and _repr_mimebundle_ #1138

ENH: Add support for capturing svg, png, and jpeg and _repr_mimebundle_ #1138

oscargus commented May 13, 2023 •

edited

lucyleeow left a comment

oscargus commented May 15, 2023

oscargus commented May 15, 2023

larsoner commented May 15, 2023

oscargus commented May 17, 2023 •

edited

larsoner commented May 17, 2023 •

edited

oscargus commented May 19, 2023

oscargus commented May 19, 2023 •

edited

oscargus commented May 19, 2023

oscargus commented May 19, 2023 •

edited

larsoner commented May 22, 2023

larsoner commented May 22, 2023

oscargus commented May 23, 2023 •

edited

oscargus commented May 23, 2023

larsoner commented May 23, 2023

larsoner May 23, 2023

larsoner commented May 23, 2023

oscargus commented Jun 3, 2023

larsoner left a comment

larsoner Jun 5, 2023

larsoner Jun 5, 2023

oscargus Jun 5, 2023

larsoner Jun 5, 2023

oscargus Jun 5, 2023

larsoner Jun 5, 2023

larsoner left a comment

	if included_mime_types and hasattr(___, '_repr_mimebundle_'):
	if included_mime_types:

ENH: Add support for capturing svg, png, and jpeg and _repr_mimebundle_ #1138

Are you sure you want to change the base?

ENH: Add support for capturing svg, png, and jpeg and _repr_mimebundle_ #1138

Conversation

oscargus commented May 13, 2023 • edited

lucyleeow left a comment

Choose a reason for hiding this comment

oscargus commented May 15, 2023

oscargus commented May 15, 2023

larsoner commented May 15, 2023

oscargus commented May 17, 2023 • edited

larsoner commented May 17, 2023 • edited

oscargus commented May 19, 2023

oscargus commented May 19, 2023 • edited

oscargus commented May 19, 2023

oscargus commented May 19, 2023 • edited

larsoner commented May 22, 2023

larsoner commented May 22, 2023

oscargus commented May 23, 2023 • edited

oscargus commented May 23, 2023

larsoner commented May 23, 2023

larsoner May 23, 2023

Choose a reason for hiding this comment

larsoner commented May 23, 2023

oscargus commented Jun 3, 2023

larsoner left a comment

Choose a reason for hiding this comment

larsoner Jun 5, 2023

Choose a reason for hiding this comment

larsoner Jun 5, 2023

Choose a reason for hiding this comment

oscargus Jun 5, 2023

Choose a reason for hiding this comment

larsoner Jun 5, 2023

Choose a reason for hiding this comment

oscargus Jun 5, 2023

Choose a reason for hiding this comment

larsoner Jun 5, 2023

Choose a reason for hiding this comment

larsoner left a comment

Choose a reason for hiding this comment

oscargus commented May 13, 2023 •

edited

oscargus commented May 17, 2023 •

edited

larsoner commented May 17, 2023 •

edited

oscargus commented May 19, 2023 •

edited

oscargus commented May 19, 2023 •

edited

oscargus commented May 23, 2023 •

edited