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

IPython Widget #5754

Merged
merged 4 commits into from May 4, 2016
Merged

IPython Widget #5754

merged 4 commits into from May 4, 2016

Conversation

blink1073
Copy link
Member

Fixes #4582. Fixes #5111. Fixes #4940. Fixes #5219. Fixes jupyter/notebook#244.

Makes the nbagg canvas an IPython Widget. mpl.js is wrapped in a define() and installed as an nbextension alongside nbagg_mpl.js.

ipython_mpl_widget_interact

@blink1073
Copy link
Member Author

I'll back out the inadvertent changes to the UAT notebook in a subsequent commit. (done).

@blink1073
Copy link
Member Author

Added toolbar.

@blink1073
Copy link
Member Author

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 backend_nbagg.py. If it does not exist, install it in the user directory. Provide a function to manually update the nb extension, and call that automatically when installing from source or from conda.

@blink1073
Copy link
Member Author

@blink1073
Copy link
Member Author

The last commit implements the idea.

@blink1073
Copy link
Member Author

The UAT passes now, except for reshow, is that still needed @tacaswell?

@blink1073
Copy link
Member Author

Interact now works with nbagg (see demo above)!

@blink1073
Copy link
Member Author

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':
Copy link
Member Author

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.

Copy link
Member

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).

@jdfreder
Copy link

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',
Copy link
Member

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?

Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member

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?

@mdboom
Copy link
Member

mdboom commented Dec 29, 2015

This is very cool. Thanks for taking this on.

Fixes #244

I don't see how this relates. Maybe wrong issue number?

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 backend_nbagg.py. If it does not exist, install it in the user directory. Provide a function to manually update the nb extension, and call that automatically when installing from source or from conda.

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?

@blink1073
Copy link
Member Author

@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 bqplot is a two-step process, you install the source, and then install the nbextension. For a system-wide install, this could be done once. A conda recipe could load the extension in the current enviroment. Wheels are not allowed to execute code on installation, they just move source around to known Python locations.

https://github.com/bloomberg/bqplot#installation

@blink1073
Copy link
Member Author

@mdboom, found it: jupyter/notebook#244

@mdboom
Copy link
Member

mdboom commented Dec 30, 2015

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) {
Copy link
Member

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. 👍

@pelson
Copy link
Member

pelson commented Jan 3, 2016

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.

@ngoldbaum
Copy link
Contributor

Just want to say that this is awesome, I'm glad progress is being made on #5111.

@jdfreder
Copy link

@mdboom I think you're interested in what I've proposed here: jupyter/notebook#116

@mdboom
Copy link
Member

mdboom commented Jan 21, 2016

@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).

@tacaswell tacaswell added this to the 2.1 (next point release) milestone Jan 29, 2016
@izaid
Copy link

izaid commented Apr 28, 2016

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?

@tacaswell
Copy link
Member

Did jupyter settle how to do package and distribute the js side of the widgets?

@izaid
Copy link

izaid commented Apr 28, 2016

@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?

@blink1073
Copy link
Member Author

@SylvainCorlay, I am actually at a loss at the moment. I'm going to have to leave this to you.

@blink1073
Copy link
Member Author

I thought I had it working at one point, but I've spent all morning on this and have not gotten it figured out.

@izaid
Copy link

izaid commented May 2, 2016

Yeah, I'm stumped here too. @SylvainCorlay, if you can drop in and save the day, that would be really cool.

@blink1073
Copy link
Member Author

Okay, I've almost got it sorted.

@blink1073
Copy link
Member Author

blink1073 commented May 2, 2016

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.

@izaid
Copy link

izaid commented May 2, 2016

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?

@izaid
Copy link

izaid commented May 4, 2016

@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.

@blink1073
Copy link
Member Author

I agree, this should be good to go.

@blink1073 blink1073 changed the title [WIP] IPython Widget IPython Widget May 4, 2016
@tacaswell
Copy link
Member

I am going to merge this on the good word of @blink1073 .

@tacaswell tacaswell merged commit 9c100c9 into matplotlib:master May 4, 2016
@tacaswell
Copy link
Member

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.

@blink1073
Copy link
Member Author

🎉

@izaid
Copy link

izaid commented May 4, 2016

This is actually a pretty big deal -- we can arrange plots in the IPython notebook now. Great work, @blink1073!

@danielballan
Copy link
Contributor

👏

@ngoldbaum
Copy link
Contributor

and callbacks that raise exceptions no longer get silently ignored 💯

@fperez
Copy link
Member

fperez commented May 5, 2016

Thanks everyone!! 🎉

@francisco-dlp francisco-dlp mentioned this pull request May 19, 2017
92 tasks
tacaswell added a commit to tacaswell/matplotlib that referenced this pull request Aug 13, 2017
…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
dopplershift added a commit that referenced this pull request Aug 14, 2017
Revert "Merge pull request #5754 from blink1073/ipython-widget"
tacaswell added a commit to tacaswell/matplotlib that referenced this pull request Oct 23, 2017
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet