Skip to content
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

Make the plot image part of the widget model #369

Closed
wants to merge 2 commits into from

Conversation

martinRenou
Copy link
Member

@martinRenou martinRenou commented Oct 7, 2021

widget_state

@github-actions
Copy link
Contributor

github-actions bot commented Oct 7, 2021

Binder 👈 Launch a binder notebook on branch martinRenou/ipympl/image_state

@martinRenou martinRenou force-pushed the image_state branch 5 times, most recently from 8403412 to c07e148 Compare October 7, 2021 11:42
@martinRenou martinRenou changed the title Make image as part of the widget model Make the plot image part of the widget model Oct 7, 2021
@martinRenou martinRenou marked this pull request as ready for review October 7, 2021 12:07
Copy link
Collaborator

@ianhi ianhi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

To make sure I understand, what's happening here is:

We stopped using the image_mode diff because we are always syncing the full image. This is what allows the frontend to hang on to the information through kernel restarts.

Blitting

If that is correct then as it stands we need to add supports_blit=False to the class so that when webagge_core flips that to True we aren't erroneously claiming that we can as the blitting there relies on the diff mode. ( https://github.com/matplotlib/matplotlib/blob/30b3bd5b97a74c6b83235a6b055dce851a154a8f/lib/matplotlib/backends/backend_webagg_core.py#L122 ) this was True in the past but went back to the False in matplotlib/matplotlib#19762

I'm a little worried that this will be limiting us to only ever doing backend blitting - which is still a performance improvement but it would be best to blit on both front and backend.

Performance?

Is there an easy way to measure our performance ( in particular of how much time we are adding by sending data through the comms)? Always sending the full image is clearly a performance degradation, but I'm not really sure by how much - hopefully it's insignificant.

(A Slight) Alternative Strategy

It may be possible to modify this a bit to both get some performance back and make full blitting easier in the future. What if instead of syncing the full image data we did the following:

  1. Keep the current diff/full image distinction
    • Could also investigate sending that information on a per draw basis in the message rather than syncing as a second trait
  2. Have a frontend only trait (non-synced) of _image which we update after doing the (maybe partial) draw on the canvas with context2d.getImageData

So on receiving new image data:

if mode full:
    clearRect
this.offscreen_context.drawImage(this.image, 0, 0);
this._image = this.offscreen_context.getImageData(0,0, this.offscreen_canvas.width, this.offscreen_canvas.height)

Then in the init we can do what you have here of checking if _image is null and if not then drawing it, the rest of the time we do the same drawImage on the canvas.

Miscellaneous Thoughts

  1. Can we disable the toolbar buttons that allow for changing the view when the widget is restored?
    • They already don't do anything but if possible we shoudl gray them out as well

2 It seems that the image/png mimetype is always overriding the widget for me in jupyterlab and I can only replicate your image in the first post in classic notebook. Is this happening for you as well?

@@ -170,6 +179,8 @@ def __init__(self, figure, *args, **kwargs):
DOMWidget.__init__(self, *args, **kwargs)
FigureCanvasWebAggCore.__init__(self, figure, *args, **kwargs)

self.set_image_mode('full')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As currently written I think you can actually eliminate this as this PR removes the ability to use the diff image mode. The image_mode is not a general backend thing, but is specific to web backends.

@martinRenou
Copy link
Member Author

martinRenou commented Oct 8, 2021

Thanks for your review!

Have a frontend only trait (non-synced) of _image which we update after doing the (maybe partial) draw on the canvas with context2d.getImageData

I was thinking of doing this indeed. I will try that.

Can we disable the toolbar buttons that allow for changing the view when the widget is restored?
They already don't do anything but if possible we shoudl gray them out as well

Yes that makes total sense. I will look into the ipywidgets code to see if there is a "closed" attribute I can use. That would also be useful for the resize handle.

It seems that the image/png mimetype is always overriding the widget for me in jupyterlab and I can only replicate your image in the first post in classic notebook. Is this happening for you as well?

Yeah I also saw that it was only working for the classic Notebook. There might be an issue in JupyterLab.

@martinRenou
Copy link
Member Author

martinRenou commented Oct 8, 2021

Have a frontend only trait (non-synced) of _image which we update after doing the (maybe partial) draw on the canvas

Giving this more thought, I don't know of a way of doing it actually. I believe ipywidgets will always sync properties. We might not have a similar option to the sync=False in Python.

EDIT: Though we might be able to overwrite the sync method https://github.com/jupyter-widgets/ipywidgets/blob/master/packages/base/src/widget.ts#L393 to prevent the sync of the _image attribute.

@martinRenou
Copy link
Member Author

Have a frontend only trait (non-synced) of _image which we update after doing the (maybe partial) draw on the canvas

I am not sure this works with nbconvert though.

@martinRenou martinRenou force-pushed the image_state branch 3 times, most recently from dacaf96 to 1a83e34 Compare October 8, 2021 13:39
@ianhi
Copy link
Collaborator

ianhi commented Oct 8, 2021

Giving this more thought, I don't know of a way of doing it actually. I believe ipywidgets will always sync properties. We might not have a similar option to the sync=False in Python.

EDIT: Though we might be able to overwrite the sync method https://github.com/jupyter-widgets/ipywidgets/blob/master/packages/base/src/widget.ts#L393 to prevent the sync of the _image attribute.

Actually I think that the frontend never synces data back unless you explicitly call model.save_changes as in https://github.com/cytoscape/ipycytoscape/blob/5e9695df32f00d47d71b053edf3bccbac3388b68/src/widget.ts#L363-L364 so since we never call that maybe we don't even need to override sync?

@martinRenou
Copy link
Member Author

Let's go ahead with #376 instead

@martinRenou martinRenou deleted the image_state branch October 18, 2021 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants