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

%matplotlib widget does not work in the scope of %display latex #33469

Closed
egourgoulhon opened this issue Mar 6, 2022 · 26 comments
Closed

%matplotlib widget does not work in the scope of %display latex #33469

egourgoulhon opened this issue Mar 6, 2022 · 26 comments

Comments

@egourgoulhon
Copy link
Member

The magic %matplotlib widget, which provides interactive Matplotlib display via the package ipympl introduced in #32069, is inoperative in the scope of %display latex:
in a Jupyter notebook the cell

%display latex
%matplotlib widget
import matplotlib.pyplot as plt
plt.plot([1, 2, 3, 4])
plt.ylabel('some numbers')
plt.show()

results in the raw output

𝙲𝚊𝚗𝚟𝚊𝚜(𝚝𝚘𝚘𝚕𝚋𝚊𝚛=𝚃𝚘𝚘𝚕𝚋𝚊𝚛(𝚝𝚘𝚘𝚕𝚒𝚝𝚎𝚖𝚜=[('𝙷𝚘𝚖𝚎', '𝚁𝚎𝚜𝚎𝚝 𝚘𝚛𝚒𝚐𝚒𝚗𝚊𝚕 𝚟𝚒𝚎𝚠', '𝚑𝚘𝚖𝚎', '𝚑𝚘𝚖𝚎'), ('𝙱𝚊𝚌𝚔', '𝙱𝚊𝚌𝚔 𝚝𝚘 𝚙𝚛𝚎𝚟𝚒𝚘𝚞𝚜 𝚟𝚒𝚎𝚠', '𝚊𝚛𝚛𝚘𝚠⎯𝚕𝚎𝚏𝚝', '𝚋𝚊𝚌𝚔'), ('𝙵𝚘𝚛𝚠𝚊𝚛𝚍', '𝙵𝚘𝚛𝚠𝚊𝚛𝚍 𝚝𝚘 𝚗𝚎𝚡𝚝 𝚟𝚒𝚎𝚠', '𝚊𝚛𝚛𝚘𝚠⎯𝚛𝚒𝚐𝚑𝚝', '𝚏𝚘𝚛𝚠𝚊𝚛𝚍'), ('𝙿𝚊𝚗', '𝙻𝚎𝚏𝚝 𝚋𝚞𝚝𝚝𝚘𝚗 𝚙𝚊𝚗𝚜, 𝚁𝚒𝚐𝚑𝚝 𝚋𝚞𝚝𝚝𝚘𝚗 𝚣𝚘𝚘𝚖𝚜\𝚗𝚡/𝚢 𝚏𝚒𝚡𝚎𝚜 𝚊𝚡𝚒𝚜, 𝙲𝚃𝚁𝙻 𝚏𝚒𝚡𝚎𝚜 𝚊𝚜𝚙𝚎𝚌𝚝', '𝚊𝚛𝚛𝚘𝚠𝚜', '𝚙𝚊𝚗'), ('𝚉𝚘𝚘𝚖', '𝚉𝚘𝚘𝚖 𝚝𝚘 𝚛𝚎𝚌𝚝𝚊𝚗𝚐𝚕𝚎\𝚗𝚡/𝚢 𝚏𝚒𝚡𝚎𝚜 𝚊𝚡𝚒𝚜', '𝚜𝚚𝚞𝚊𝚛𝚎⎯𝚘', '𝚣𝚘𝚘𝚖'), ('𝙳𝚘𝚠𝚗𝚕𝚘𝚊𝚍', '𝙳𝚘𝚠𝚗𝚕𝚘𝚊𝚍 𝚙𝚕𝚘𝚝', '𝚏𝚕𝚘𝚙𝚙𝚢⎯𝚘', '𝚜𝚊𝚟𝚎⎯𝚏𝚒𝚐𝚞𝚛𝚎')]))

instead of the interactive Matplotlib window. Replacing %display latex by %display plain makes the cell work fine.
See also the test notebook below.

This is problematic for Jupyterlab, where the magic %matplotlib notebook is not supported and must be replaced by %matplotlib widget.


Notebook for testing display in Jupyter:

test_display.ipynb.

Note that it lies in a git repository, so feel free to amend it by sending a pull request to SageMathTest.

Depends on #32069

CC: @kwankyu

Component: notebook

Keywords: jupyterlab, matplotlib widget

Author: Nicolas M. Thiéry

Branch/Commit: d8dd0db

Reviewer: Eric Gourgoulhon, Kwankyu Lee

Issue created by migration from https://trac.sagemath.org/ticket/33469

@egourgoulhon

This comment has been minimized.

@egourgoulhon
Copy link
Member Author

Changed keywords from none to jupyterlab, matplotlib widget

@nthiery
Copy link
Contributor

nthiery commented Mar 9, 2022

comment:3

I hit presumably the same problem this morning in class with
students that wanted to build widgets in Sage.

How to reproduce

%display latex
import ipywidgets
ipywidgets.IntSlider()

outputs a string representation instead of the slider:

𝙸𝚗𝚝𝚂𝚕𝚒𝚍𝚎𝚛(𝚟𝚊𝚕𝚞𝚎=𝟶)

Proposed fix

Sage's formatter has a list of "Python native types"
that should be left alone during display. This list contains
interactive, the type of objects produced by @interact. This
could be generalized to any jupyter widget (type ipywigets.Widget),
which should include the widgets produced by ipyml.

Temporary workaround

Apply the proposed fix by monkey patching:

sage.repl.display.formatter.IPYTHON_NATIVE_TYPES += (ipywidgets.Widget,)

@nthiery
Copy link
Contributor

nthiery commented Mar 9, 2022

comment:4

Eric: does the workaround work for ipyml?

Cheers,

@egourgoulhon
Copy link
Member Author

comment:5

Replying to @nthiery:

Eric: does the workaround work for ipyml?

Yes, absolutely:

import ipympl
sage.repl.display.formatter.IPYTHON_NATIVE_TYPES += (ipympl.backend_nbagg.Canvas,)

fixes the issue. Thank you Nicolas!

@egourgoulhon
Copy link
Member Author

comment:6

Replying to @egourgoulhon:

Replying to @nthiery:

Eric: does the workaround work for ipyml?

Yes, absolutely:

import ipympl
sage.repl.display.formatter.IPYTHON_NATIVE_TYPES += (ipympl.backend_nbagg.Canvas,)

fixes the issue. Thank you Nicolas!

Actually, this specific addition is not even necessary: as you pointed out

import ipywidgets
sage.repl.display.formatter.IPYTHON_NATIVE_TYPES += (ipywidgets.Widget,)

is more generic and suffices to fix the issue with ipympl.

@nthiery
Copy link
Contributor

nthiery commented Mar 9, 2022

comment:7

Great. So the next step is to implement the fix directly in Sage.

I might not be able to do it in the next days, so anyone feel free
to take the lead :-)

@nthiery
Copy link
Contributor

nthiery commented Mar 9, 2022

@nthiery
Copy link
Contributor

nthiery commented Mar 9, 2022

comment:9

Miracle: upgrading my sage was quick, and git trac still functional on my laptop.
First submission in a loooong time ...


New commits:

7d93b2533469: fix %display latex for all Jupyter widgets

@nthiery
Copy link
Contributor

nthiery commented Mar 9, 2022

Commit: 7d93b25

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 9, 2022

Changed commit from 7d93b25 to d8dd0db

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 9, 2022

Branch pushed to git repo; I updated commit sha1. New commits:

d8dd0db33469: attempt at adding a test

@nthiery
Copy link
Contributor

nthiery commented Mar 9, 2022

comment:11

Well, needs review I guess.

I tried adding a test, but actually the test pass even before the patch.
Ideas anyone for a better test?

@kwankyu
Copy link
Collaborator

kwankyu commented Mar 10, 2022

comment:12

Replying to @nthiery:

I tried adding a test, but actually the test pass even before the patch.
Ideas anyone for a better test?

I have no idea, either. But if the test does not test the patch, then I think it is of no use.

On the other hand, any change in the output system may cause unexpected side effects which are detected only by human eyes. It seems that the best we can do is to check a good many examples with the patch. There is another ticket #11362 that needs the same scrutiny.

@kwankyu
Copy link
Collaborator

kwankyu commented Mar 10, 2022

comment:13

Replying to @kwankyu:

Replying to @nthiery:
It seems that the best we can do is to check a good many examples with the patch.

@Eric. I suggest making a "standard proof sheet" from the notebooks here

#32208 comment:13

@nthiery
Copy link
Contributor

nthiery commented Mar 10, 2022

comment:14

But if the test does not test the patch, then I think it is of no use.

Agreed. I only included it to be precise about what I tried, in the
hope someone would find something better. This commit should indeed
be reverted / popped before merging.

@egourgoulhon
Copy link
Member Author

comment:15

Replying to @kwankyu:

@Eric. I suggest making a "standard proof sheet" from the notebooks here

#32208 comment:13

I've already updated test_display_latex.ipynb, but I'll prepare a better one soon, including threejs animations and so on...

@egourgoulhon
Copy link
Member Author

comment:16

Replying to @nthiery:

But if the test does not test the patch, then I think it is of no use.

Agreed. I only included it to be precise about what I tried, in the
hope someone would find something better. This commit should indeed
be reverted / popped before merging.

I don't see any simple doctest either, so yes, maybe the best is to revert this.

@egourgoulhon
Copy link
Member Author

comment:17

Replying to @egourgoulhon:

Replying to @kwankyu:

@Eric. I suggest making a "standard proof sheet" from the notebooks here

#32208 comment:13

I've already updated test_display_latex.ipynb, but I'll prepare a better one soon, including threejs animations and so on...

Here it is : test_display.ipynb.
Note that it lies in a git repository, so feel free to amend it by sending a pull request to SageMathTest.

It would be nice to have this ticket merged in Sage 9.6. IMHO, it is ready to go. Simply maybe the test mentioned in comment:11 could be removed, given that the actual test is the "human-eyes" one, e.g. via the above notebook.

@kwankyu

This comment has been minimized.

@kwankyu
Copy link
Collaborator

kwankyu commented Mar 23, 2022

comment:18

Replying to @egourgoulhon:

It would be nice to have this ticket merged in Sage 9.6. IMHO, it is ready to go. Simply maybe the test mentioned in comment:11 could be removed, given that the actual test is the "human-eyes" one, e.g. via the above notebook.

+1 for positive review.

@kwankyu
Copy link
Collaborator

kwankyu commented Mar 23, 2022

comment:19

Replying to @egourgoulhon:

Here it is : test_display.ipynb...

Thank you!

@egourgoulhon
Copy link
Member Author

comment:20

OK, let's move on. Thank you Nicolas for the fix!

@egourgoulhon
Copy link
Member Author

Reviewer: Eric Gourgoulhon, Kwankyu Lee

@egourgoulhon
Copy link
Member Author

Author: Nicolas M. Thiéry

@vbraun
Copy link
Member

vbraun commented Mar 30, 2022

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants