-
-
Notifications
You must be signed in to change notification settings - Fork 7.5k
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
Adding png image return for inline backend figures with _repr_html_ #16788
Adding png image return for inline backend figures with _repr_html_ #16788
Conversation
lib/matplotlib/figure.py
Outdated
@@ -375,6 +375,15 @@ def _repr_html_(self): | |||
from matplotlib.backends import backend_webagg | |||
return backend_webagg.ipython_inline_display(self) | |||
|
|||
if mpl.rcParams['backend'] == 'module://ipykernel.pylab.backend_inline': |
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 should (likely?) check the canvas type (as for webagg), because rcParams["backend"] may change?
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.
That makes sense - should we have a figure output for all backends? And just choose the image format based on that backend?
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.
Instead of adding more coditional paths here we should make this defer to a _repr_html_
on the canvas.
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.
@tacaswell Would adding a single _repr_html_
to backend_bases.FigureCanvaseBase
be enough?
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.
No, because for the interactive backends we do not want to replace them with a dead png.
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.
But this would only get triggered if a figure was the output of the last line of a cell in a notebook, which would (likely) only be done intentionally.
Awesome, thanks for opening this issue! |
lib/matplotlib/figure.py
Outdated
@@ -375,6 +375,15 @@ def _repr_html_(self): | |||
from matplotlib.backends import backend_webagg | |||
return backend_webagg.ipython_inline_display(self) | |||
|
|||
if mpl.rcParams['backend'] == 'module://ipykernel.pylab.backend_inline': |
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.
Instead of adding more coditional paths here we should make this defer to a _repr_html_
on the canvas.
I changed the approach completely and added a In Figure, the |
lib/matplotlib/backend_bases.py
Outdated
|
||
fmt = self.get_default_filetype() | ||
dpi = self.figure.dpi | ||
if fmt == 'retina': |
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.
is there really a backend that defines such a default filetype?
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.
FigureCanvasBase
defaults to rcParams['savefig.format']
. svg, pdf, pgf, ps all have this method implemented.
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.
Yes, but the format won't be 'retina'.
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.
Thank you for this. I see what I need to do. Get the ipython shell and check for retina as the figure_format. I would also need to get other settings like bbox_inches from there.
Regarding retina - it implicitly doubles the DPI, but this gives a false impression of what a saved image will look like. I think it would be better to change the rcparams. Even better would be to have a backend titled 'inline_real' and set the DPI to the DPI of the monitor (and adjust the sizes of all fonts, lines, tick lines, etc.. accordingly.).
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.
I am somewhat familiar with the problems of retina (well, on a theoretical PoV). My point is just that get_default_filetype() never returns (AFAIK) "retina"; that info is simply stashed elsewhere.
lib/matplotlib/backend_bases.py
Outdated
|
||
mimetype_dict = {'png': 'image/png', 'svg': 'image/svg+xml', | ||
'jpg': 'image/jpeg', 'pdf': 'application/pdf'} | ||
if fmt not in mimetype_dict: |
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.
falling back to png here seems wrong/too late (it could well not be a png file, indeed).
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.
As of now, running %matplotlib inline uses default figure_format as png. So regardless of the backend, a png will be produced.
} | ||
|
||
bytes_io = io.BytesIO() | ||
self.print_figure(bytes_io, **kw) |
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.
you can just pass the kwargs here (having a separate dict doesn't seem to buy much)
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.
and you need to force the format here. again, bytes_io could easily contain a postscript image here, for example.
lib/matplotlib/backend_bases.py
Outdated
self.print_figure(bytes_io, **kw) | ||
raw_bytes = bytes_io.getvalue() | ||
|
||
mimetype_dict = {'png': 'image/png', 'svg': 'image/svg+xml', |
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.
see #14218 for how to get the mapping from the stdlib
Made more changes to Currently, when retina is chosen as the figure format with The All backends use Backend |
Ah, I missed the fact that ipython had an additional "retina" format. Ugh. |
lib/matplotlib/backend_bases.py
Outdated
@@ -1595,6 +1595,73 @@ def __init__(self, figure): | |||
self.toolbar = None # NavigationToolbar2 will set me | |||
self._is_idle_drawing = False | |||
|
|||
def _repr_html_(self): | |||
if not self.figure.axes and not self.figure.lines: | |||
return |
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.
What if the figure only contains a Rectangle an arrow and a Text?
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.
True, I copied that from elsewhere. Is there a clean way to determine if the figure has changed at all?
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.
Check https://matplotlib.org/faq/howto_faq.html#check-whether-a-figure-is-empty
However, maybe even completely empty figures should return some html? After all, you get what you ask for, right?
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.
What do you mean "changed"?
I agree with @ImportanceOfBeingErnest if we are going to have a repr, it should always show something even if that something is an empty image.
lib/matplotlib/backend_bases.py
Outdated
is_retina = False | ||
|
||
import IPython | ||
ip = IPython.get_ipython() |
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 would fail whenever IPython is not available.
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.
Wouldn't _repr_html_
only get triggered for ipython?
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.
I guess IPython invented this; but in principle anything can try to call such method if it's available.
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.
The other way you can check this is if 'IPython' in sys.modules
. That way if the user has not previously imported IPython we won't do it for them.
This seems to pretty much reimplement the Ipykernel inline backend into matplotlib. I'm not convinced that this is the right approach, because it uses the IPython configuration system to determine the right parameters and thereby introduces a dependency on that config system which I don't think should be there. What about adding a |
@ImportanceOfBeingErnest The point of this is to work with jupyter notebooks whenever a figure is the last line of a cell without the need for The only reason IPython is accessed is to check whether the display is retina and what If you had a |
I think my problem is really about the two-way dependency introduced here. If IPython/IPykernel decides to introduce a new figure format But if we don't, then people will complain that the figures they produce with the IPykernel inline backend look different from the ones they create via matplotlib's |
Good point. It would be nice if ipython was made completely independent from matplotlib and dropped all of its code that had to do with it. matplotlib could then handle everything with the importing of pyplot and |
lib/matplotlib/backend_bases.py
Outdated
dpi = dpi * 2 | ||
fmt = self.get_default_filetype() | ||
else: | ||
bbox_inches = 'tight' # how to let user choose self.figure.bbox_inches? |
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 just should not be passed or set to None. That IPython inline defaults to 'tight'
has caused a tremendous amount of confusion over the years.
I again agree with @ImportanceOfBeingErnest , the default implementation on the canvas base-class should not special case the inline backend and IPyhon's configuration system (just embedding a png at the native size would be fine). The support for that could be done in a companion PR into IPython to move their logic to the correct place. It seems fair for people who want the fancier controls from IPython have to use the IPython sourced backend. There will also need to be a companion PR into ipympl so that we correctly defer to their canvas as well. I am very 👍 on this PR in principle, but the technical interaction between Matplotlib and IPython/jupyter has always been complicated and we need to make sure that we are making it simpler for the devs while making it more streamlined for the users. |
Thanks @tacaswell for the feedback. I've changed approaches again and now defer all html output to ipython if Otherwise, the default canvas format gets returned as HTML. Essentially, this method will only be useful for those who don't run |
ib_list = [c for c in ip.configurables | ||
if 'InlineBackend' in type(c).__name__] | ||
if ib_list: | ||
return |
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 dived into IPython itself. It looks like we are also storing the sate on a function attribute (!!!).
In [1]: from IPython.core.pylabtools import configure_inline_support
In [2]: configure_inline_support.current_backend
---------------------------------------------------------------------------
AttributeError Traceback (most recent call last)
<ipython-input-2-eec19e858e54> in <module>
----> 1 configure_inline_support.current_backend
AttributeError: 'function' object has no attribute 'current_backend'
In [3]: %matplotlib qt
In [4]: configure_inline_support.current_backend
Out[4]: 'other'
In [5]: %matplotlib inline
In [6]: configure_inline_support.current_backend
Out[6]: 'inline'
Not sure it is better though.
Definitively the separation IPython/Matplotlib is not great, and I'm happy to try to get better behavior on the IPython codebase. The concern I have with I don't like having to special case matplotlib either, and admittedly there is no really good way to know wether we are in inline mode or not. At some points, we likely want to sit-down for a couple of hours list the current behavior and plan for a progressive cleanup over the next few years. |
Heh, I'm about to submit a post on the matplotlib discourse stating how great this would be :) Regarding the double display: Currently, I experience the double display whenever I have a Agreed, the current behavior is pretty convoluted and I think can be greatly simplified. |
Closing in lieu of #17891, but feel free to reopen if I've misunderstood... |
PR Summary
This will enable figures to be displayed in a jupyter notebook without ever having to run
%matplotlib inline
orimport matplotlib.pyplot
.The initial issue was created here #16782.
I'm sure the code will need to be modified quite a bit to work with more cases, but I wanted to get the ball started on this important issue for me.
I don't know how to test this. Are there special tests for jupyter notebook output?
PR Checklist