remove unnecessary traitlets to fix savefig and display(fig) errors #264

ianhi · 2020-09-08T15:57:53Z

fixes: #252

I also preemptively removed several other attributes that aren't used by the js and so do not need to be traitlets.

I left force_full, _current_image_mode and _dpi_ratio as these have code of the js side that could use them, although currently all of the values for those are hardcoded in the js and the traitlets aren't used.

attn: @martinRenou @tacaswell

Could a release be made after merging this?

these aren't used by the js side and making them traitlets may cause errors with high enough versions of matplotlib

martinRenou

Thanks a lot for opening a PR! I can make a release after this is merged.

martinRenou · 2020-09-08T16:00:17Z

ipympl/backend_nbagg.py

-    _button = Any()
-    _key = Any()
-    _lastx = Any()
-    _lasty = Any()


Just wondering if those are used in core Matplotlib, do you know @tacaswell?

Yes, we use them for preventing re-entrant draws (_is_idle_drawing and _is_saving) and assembling the composite events that eventually propagates through our event system (because the keyboard and mouse events come in separate and we merge them).

So is it safe removing them?

The canvas still has all of these attributes that it inherits from webagg. So this runs without throwing an error:

for attr in ['_lastx', '_lasty', '_button', '_key', '_is_saving', '_is_idle_drawing']: assert hasattr(fig.canvas, attr)

so I think it is safe to remove them.

martinRenou · 2020-09-08T16:05:09Z

@ianhi should I update the Matplotlib requirement when making the release?

ianhi · 2020-09-08T16:07:02Z

I don't think that's necessary? I think this will only make it compatible with more versions of matplotlib rather than fewer

martinRenou · 2020-09-08T16:08:34Z

Good! Thanks, I was just wondering if that was a breaking change for older Matplotlib

ianhi · 2020-09-08T16:08:55Z

Actually hang on. I may have introduced an error with mapltotlib ==3.2

ianhi · 2020-09-08T16:15:14Z

On 3.2.0 I get this error for display(fig)

---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
~/anaconda3/envs/ipympl/lib/python3.8/site-packages/matplotlib/backend_bases.py in _wait_cursor_for_draw_cm(self)
   2772             try:
-> 2773                 self.set_cursor(cursors.WAIT)
   2774                 yield

~/anaconda3/envs/ipympl/lib/python3.8/site-packages/matplotlib/backends/backend_webagg_core.py in set_cursor(self, cursor)
    380         if cursor != self.cursor:
--> 381             self.canvas.send_event("cursor", cursor=cursor)
    382         self.cursor = cursor

~/anaconda3/envs/ipympl/lib/python3.8/site-packages/matplotlib/backends/backend_webagg_core.py in send_event(self, event_type, **kwargs)
    345     def send_event(self, event_type, **kwargs):
--> 346         self.manager._send_event(event_type, **kwargs)
    347 

AttributeError: 'NoneType' object has no attribute '_send_event'

During handling of the above exception, another exception occurred:

AttributeError                            Traceback (most recent call last)
~/anaconda3/envs/ipympl/lib/python3.8/site-packages/IPython/core/formatters.py in __call__(self, obj)
    339                 pass
    340             else:
--> 341                 return printer(obj)
    342             # Finally look for special method names
    343             method = get_real_method(obj, self.print_method)

~/anaconda3/envs/ipympl/lib/python3.8/site-packages/IPython/core/pylabtools.py in <lambda>(fig)
    246 
    247     if 'png' in formats:
--> 248         png_formatter.for_type(Figure, lambda fig: print_figure(fig, 'png', **kwargs))
    249     if 'retina' in formats or 'png2x' in formats:
    250         png_formatter.for_type(Figure, lambda fig: retina_figure(fig, **kwargs))

~/anaconda3/envs/ipympl/lib/python3.8/site-packages/IPython/core/pylabtools.py in print_figure(fig, fmt, bbox_inches, **kwargs)
    130         FigureCanvasBase(fig)
    131 
--> 132     fig.canvas.print_figure(bytes_io, **kw)
    133     data = bytes_io.getvalue()
    134     if fmt == 'svg':

~/anaconda3/envs/ipympl/lib/python3.8/site-packages/matplotlib/backend_bases.py in print_figure(self, filename, dpi, facecolor, edgecolor, orientation, format, bbox_inches, **kwargs)
   2061             if bbox_inches:
   2062                 if bbox_inches == "tight":
-> 2063                     renderer = _get_renderer(
   2064                         self.figure,
   2065                         functools.partial(

~/anaconda3/envs/ipympl/lib/python3.8/site-packages/matplotlib/backend_bases.py in _get_renderer(figure, print_method)
   1532     with cbook._setattr_cm(figure, draw=_draw):
   1533         try:
-> 1534             print_method(io.BytesIO())
   1535         except Done as exc:
   1536             figure._cachedRenderer, = exc.args

~/anaconda3/envs/ipympl/lib/python3.8/site-packages/matplotlib/backends/backend_agg.py in print_png(self, filename_or_obj, metadata, pil_kwargs, *args, **kwargs)
    512         }
    513 
--> 514         FigureCanvasAgg.draw(self)
    515         if pil_kwargs is not None:
    516             from PIL import Image

~/anaconda3/envs/ipympl/lib/python3.8/site-packages/matplotlib/backends/backend_agg.py in draw(self)
    388         self.renderer = self.get_renderer(cleared=True)
    389         # Acquire a lock on the shared font cache.
--> 390         with RendererAgg.lock, \
    391              (self.toolbar._wait_cursor_for_draw_cm() if self.toolbar
    392               else nullcontext()):

~/anaconda3/envs/ipympl/lib/python3.8/contextlib.py in __enter__(self)
    111         del self.args, self.kwds, self.func
    112         try:
--> 113             return next(self.gen)
    114         except StopIteration:
    115             raise RuntimeError("generator didn't yield") from None

~/anaconda3/envs/ipympl/lib/python3.8/site-packages/matplotlib/backend_bases.py in _wait_cursor_for_draw_cm(self)
   2774                 yield
   2775             finally:
-> 2776                 self.set_cursor(self._lastCursor)
   2777         else:
   2778             yield

~/anaconda3/envs/ipympl/lib/python3.8/site-packages/matplotlib/backends/backend_webagg_core.py in set_cursor(self, cursor)
    379     def set_cursor(self, cursor):
    380         if cursor != self.cursor:
--> 381             self.canvas.send_event("cursor", cursor=cursor)
    382         self.cursor = cursor
    383 

~/anaconda3/envs/ipympl/lib/python3.8/site-packages/matplotlib/backends/backend_webagg_core.py in send_event(self, event_type, **kwargs)
    344 
    345     def send_event(self, event_type, **kwargs):
--> 346         self.manager._send_event(event_type, **kwargs)
    347 
    348 

AttributeError: 'NoneType' object has no attribute '_send_event'

but this is then fixed on 3.2.1 so I think that it was a matplotlib error not an ipympl issue.

lgtm-com · 2020-09-08T16:16:25Z

This pull request introduces 3 alerts when merging f8e83f7 into 58bdb1e - view on LGTM.com

new alerts:

2 for Multiple calls to __init__ during object initialization
1 for Unused import

kaedonkers · 2020-09-09T14:20:48Z

I just found this bug this morning - great to see a fix on the way!

martinRenou · 2020-09-10T07:57:47Z

2 for Multiple calls to init during object initialization

I don't get what those two new alerts are, see https://lgtm.com/projects/g/matplotlib/ipympl/rev/pr-216ee8606156138123b45a51f47712789632b603

NavigationToolbar2.__init__ may be called multiple times during initialization.

Any idea @ianhi ?

ianhi · 2020-09-10T15:43:46Z

I'm confused as well, especially because I didn't modify the toolbar at all. I think traitlets 5 was released between this and the last time the lgtm ran on this repo so maybe that's whats going on? I can't figure out how to view the build logs for pull requests on lgtm.com so its tricky to check, but in a new conda env pip install ipympl pulls in traitlets==5.0.4 for me so it's likely that lgtm picked that up. Maybe we should try requiring traitlets <5 and see if that fixes it.

martinRenou · 2020-09-10T15:46:34Z

Good catch that might be it, although if it's static analysis it should not rely on dependency versions.. Let's merge your PR as it is.

ianhi · 2020-09-10T16:33:26Z

🎉 hooray! For the new release when updating the readme should there be a new column that lists matplotlib versions?

martinRenou · 2020-09-10T16:47:56Z

Good idea, thanks! I'll do the release first thing tomorrow.

martinRenou · 2020-09-11T07:06:18Z

Released 0.5.8, it's available from pypi and soon from conda

remove unnecessary traitlets

f8e83f7

these aren't used by the js side and making them traitlets may cause errors with high enough versions of matplotlib

martinRenou reviewed Sep 8, 2020

View reviewed changes

tacaswell approved these changes Sep 8, 2020

View reviewed changes

martinRenou merged commit 1f71bfd into matplotlib:master Sep 10, 2020

ianhi deleted the new-traitlets-test branch September 10, 2020 16:31

ianhi mentioned this pull request Apr 15, 2021

Handle new FigureCanvasBase.device_pixel_ratio. #317

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove unnecessary traitlets to fix savefig and display(fig) errors #264

remove unnecessary traitlets to fix savefig and display(fig) errors #264

ianhi commented Sep 8, 2020

martinRenou left a comment

martinRenou Sep 8, 2020 •

edited

Loading

tacaswell Sep 8, 2020

martinRenou Sep 9, 2020

ianhi Sep 9, 2020

martinRenou commented Sep 8, 2020

ianhi commented Sep 8, 2020

martinRenou commented Sep 8, 2020

ianhi commented Sep 8, 2020

ianhi commented Sep 8, 2020

lgtm-com bot commented Sep 8, 2020

kaedonkers commented Sep 9, 2020

martinRenou commented Sep 10, 2020

ianhi commented Sep 10, 2020

martinRenou commented Sep 10, 2020

ianhi commented Sep 10, 2020

martinRenou commented Sep 10, 2020

martinRenou commented Sep 11, 2020

remove unnecessary traitlets to fix savefig and display(fig) errors #264

remove unnecessary traitlets to fix savefig and display(fig) errors #264

Conversation

ianhi commented Sep 8, 2020

martinRenou left a comment

Choose a reason for hiding this comment

martinRenou Sep 8, 2020 • edited Loading

Choose a reason for hiding this comment

tacaswell Sep 8, 2020

Choose a reason for hiding this comment

martinRenou Sep 9, 2020

Choose a reason for hiding this comment

ianhi Sep 9, 2020

Choose a reason for hiding this comment

martinRenou commented Sep 8, 2020

ianhi commented Sep 8, 2020

martinRenou commented Sep 8, 2020

ianhi commented Sep 8, 2020

ianhi commented Sep 8, 2020

lgtm-com bot commented Sep 8, 2020

kaedonkers commented Sep 9, 2020

martinRenou commented Sep 10, 2020

ianhi commented Sep 10, 2020

martinRenou commented Sep 10, 2020

ianhi commented Sep 10, 2020

martinRenou commented Sep 10, 2020

martinRenou commented Sep 11, 2020

martinRenou Sep 8, 2020 •

edited

Loading