-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
IPython Widget #5754
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
IPython Widget #5754
Conversation
I'll back out the inadvertent changes to the UAT notebook in a subsequent commit. (done). |
Added toolbar. |
We can't install the nb extension from a wheel, so I'd recommend the following: check for the existence of the nb extension when importing |
The last commit implements the idea. |
The UAT passes now, except for |
Interact now works with nbagg (see demo above)! |
Note to self: add an explicit "export" button instead of spitting out an image automatically when the widget is closed. (done). |
@@ -2148,7 +2148,8 @@ def print_figure(self, filename, dpi=None, facecolor='w', edgecolor='w', | |||
origfacecolor = self.figure.get_facecolor() | |||
origedgecolor = self.figure.get_edgecolor() | |||
|
|||
self.figure.dpi = dpi | |||
if dpi != 'figure': |
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 avoids an error I saw.
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.
FWIW: This was fixed recently by #5720 (in the exact same way).
Cool! |
@@ -98,6 +103,7 @@ def connection_info(): | |||
'zoom_to_rect': 'fa fa-square-o icon-check-empty', | |||
'move': 'fa fa-arrows icon-move', | |||
'download': 'fa fa-floppy-o icon-save', | |||
'export': 'fa fa-file-picture-o icon-picture', |
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.
Can you elaborate on what "export" does? It inserts a static image into the notebook?
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.
Why is this preferable to the automatic replacement with a static image upon saving? Won't users often forget to push this button?
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, it inserts a static image into the notebook. I'm looking at this from the point of view where the canvas is one of many widgets in a gui, and you want explicit control over export behavior.
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.
Maybe we should think about the name. Export makes me think of "save" - I guess this is a kind of "snapshot" type behaviour, right?
This is very cool. Thanks for taking this on.
I don't see how this relates. Maybe wrong issue number?
Can you elaborate on this? It seems to add a lot of complexity for reasons that are lost to me, as someone who hasn't really followed Jupyter/IPython widgets. How does this work in the context where the matplotlib package is installed system-wide? |
@mdboom, I removed the reference to 244, I can't find the issue I had meant to put there, it was referring to the fact that you could not "print" events to the notebook (as is now shown in the demo above). For reference, installing |
@mdboom, found it: jupyter/notebook#244 |
Sorry, I still don't understand the need for the complexity of installing the Python code and Javascript code separately. Can't we get to the JavaScript code from where it's installed under the matplotlib package (as we do now)? |
|
||
mpl.find_output_cell = function(html_output) { |
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'm glad to see the back of this function. 👍
Untested, but 👍 in principle. Now that we are benefiting from hooking into the widget framework, do we also get a plot when the notebook page is refreshed? It might be worth going through the UATs and adding a refresh test. |
Just want to say that this is awesome, I'm glad progress is being made on #5111. |
@mdboom I think you're interested in what I've proposed here: jupyter/notebook#116 |
@jdfreder: Thanks for the pointer. Indeed, it looks at first glance that jupyter/notebook#116 is moving in the direction that would really help us here. (It also helps me understand all the gory details and opposing requirements much better). |
Hey all. And directly pinging @blink1073 @SylvainCorlay @mdboom. I'd really like to see this get merged into Matplotlib. I understand there are issues on the Jupyter side that may be holding this up. I can devote some of my time (on the order of days, if necessary) to get this resolved. Can someone tell me what needs to be done and what I can do to move things forward? |
Did jupyter settle how to do package and distribute the js side of the widgets? |
@tacaswell I don't know the answer to that, and I'm not sure who to ask. Pinging @SylvainCorlay again (sorry), because I think he knows. I just want to volunteer my time and effort to finish this, and need some direction on what needs to be done. If the packaging isn't solved, can we not just merge this as is and have a follow-up PR for that? |
@SylvainCorlay, I am actually at a loss at the moment. I'm going to have to leave this to you. |
I thought I had it working at one point, but I've spent all morning on this and have not gotten it figured out. |
Yeah, I'm stumped here too. @SylvainCorlay, if you can drop in and save the day, that would be really cool. |
Okay, I've almost got it sorted. |
The latest change allows it to work, but the extension needs to installed on a per environment basis and enabled for the user in order to allow different versions per environment IIUC. |
I just confirmed that it works on my local version of Jupyter 4.2. This is really great, thanks for picking this up again! Re: Local installation. Isn't that the model for Jupyter now? |
@tacaswell @blink1073 So, I've been using this branch for a few days now, and my understanding is it is done. Any chance we can get this merged into master? I wouldn't mind building a few things on top of it, but I'd rather do that after merge in separate, unrelated PRs. |
I agree, this should be good to go. |
I am going to merge this on the good word of @blink1073 . |
If people were doing things in the notebook which relied on the previous version of the js we shipped we will re-address adding it back. |
🎉 |
This is actually a pretty big deal -- we can arrange plots in the IPython notebook now. Great work, @blink1073! |
👏 |
and callbacks that raise exceptions no longer get silently ignored 💯 |
Thanks everyone!! 🎉 |
…get" This reverts commit 9c100c9, reversing changes made to 6bed1be. The reason for doing this is that ipywidgets is still moving much too fast for Matplotlib to keep up with. The original work done in matplotlib#5754 was done against ipywidgets v4, as of now the final touches are being put on v7. We are reverting back to our old inject-into-the-DOM method from `nbagg`, which is what is used by `%matplotlib notebook`. For embedding as a 'proper' widget use ipympl (github.com/matplotlib/jupyter-wigets) which will release on a schedule that matches the upstream jupyter ecosystem. closes matplotlib#7695
Revert "Merge pull request #5754 from blink1073/ipython-widget"
These files were moved as part of matplotlib#5754 which was reverted in matplotlib#9027, however matplotlib#6370 fixed the paths, but was not also reverted. Putting the js files in sub-directory makes sense, move the files rather than revert the changes in matplotlib#6370. fixes matplotlib#9380
Fixes #4582. Fixes #5111. Fixes #4940. Fixes #5219. Fixes jupyter/notebook#244.
Makes the nbagg canvas an IPython Widget.
mpl.js
is wrapped in adefine()
and installed as annbextension
alongsidenbagg_mpl.js
.