-
Notifications
You must be signed in to change notification settings - Fork 197
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
ENH: Add support for capturing svg, png, and jpeg and _repr_mimebundle_ #1138
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
Thanks! I'll try to add tests and documentation. |
c827db0
to
e2a1592
Compare
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. |
You could make custom classes with the |
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? It may be that this can be added on later, depending on how hard it is... |
I would start out by writing the files to disk just like current scrapers, then adding them as |
2e10238
to
19d569e
Compare
OK, so now it writes files. Also, I added support for 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. |
The content of It is hard to say how many more packages can be supported by this. A quick search got ~1200 hits for However, as I personally rely on SymPy and GraphViz quite a bit, it is a real game changer for me. :-) |
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? |
6c1901e
to
243069d
Compare
Error on CircleCI:
One way to avoid this would be to wrap the SVG output in |
... but I should add that the code otherwise looks good, and this output shows things nicely: So once we can make CIs happy we should be good |
246a7a1
to
1d282e5
Compare
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 |
b04e172
to
989934d
Compare
This is the current output and source to the test errors: As seen, the |
For now then let's just detect if it's SVG and wrap it in a |
sphinx_gallery/scrapers.py
Outdated
@@ -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): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove sg_image
for now
Locally you could check this, but let's use CircleCI instead. The RST files are actually saved to disk and you can look: You can see the problematic line:
In particular note the leading space |
9fe1bd4
to
d044ca4
Compare
I got it fully working using sphinx-gallery/sphinx_gallery/directives.py Lines 186 to 188 in 1e825be
Had to install pipwin and from there cairocffi to get the Windows tests passing. But now it seems to work properly: Not sure if this is an acceptable solution for now or if it may break something else? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few minor comments otherwise LGTM!
@@ -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): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might as well add these because it's easy
svg_image=False): | |
*, svg_image=False): |
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_'): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This hasattr
is unnecessary, the try
below would catch the AttributeError
below anyway. Might as well remove it
if included_mime_types and hasattr(___, '_repr_mimebundle_'): | |
if included_mime_types: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
# Used for SVGs | ||
SVG_IMAGE = """ | ||
.. image:: /%s | ||
:alt: %s | ||
:class: sphx-glr-single-img | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think the space is a bug and can safely be removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few minor comments otherwise LGTM!
Closes #1136
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.)