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

Fix _repr_ of graphics objects #14469

Closed
vbraun opened this issue Apr 20, 2013 · 22 comments
Closed

Fix _repr_ of graphics objects #14469

vbraun opened this issue Apr 20, 2013 · 22 comments

Comments

@vbraun
Copy link
Member

vbraun commented Apr 20, 2013

Graphical output of plots (or other graphics objects) is hooked into _repr_(). Obviously, Python doesn't expect repr to have side effects so we get fun stuff like

sage: g = Graphics()
sage: g?

showing a plot in addition to the help. Or (g,g) opening two plot output windows if used on the command line. You don't want to try [g]*100

This patch moves the decision logic into the displayhook, and makes repr always return a string representation as it should.

Also, deprecate the show_output() function. Wat?

Depends on #14203
Depends on #14266
Depends on #14471
Depends on #15016

CC: @ppurka

Component: graphics

Author: Volker Braun

Reviewer: Travis Scrimshaw

Merged: sage-5.12.beta1

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

@vbraun

This comment has been minimized.

@vbraun
Copy link
Member Author

vbraun commented Apr 20, 2013

Dependencies: #14203

@vbraun
Copy link
Member Author

vbraun commented Apr 20, 2013

Author: Volker Braun

@kcrisman
Copy link
Member

comment:5

I've totally done something analogous to [g]*100. Making lists of plots is useful, but the previous behavior was not good, and no one should have been relying on it.

I don't have time to learn about displayhook now, but the rest looks good, assuming it works properly in the notebook.

Unless... you don't suppose someone would actually want to be able to change the default behavior of (say) [g]*100 to show all these graphics in addition to printing the list? Just a crazy thought.

@vbraun
Copy link
Member Author

vbraun commented Apr 22, 2013

comment:6

The IMHO only remaining problem is that the patch triggers what looks like a Python bug, see #14471.

@tscrim
Copy link
Collaborator

tscrim commented Apr 25, 2013

comment:7

Two things on the current patch:

  • Is there a reason why you're importing DOCTEST_MODE in displayhook.py? You don't seem to be using it.
  • The method _graphics_() in sage_input.py will need a doctest.

Also I think if someone wants [g]*100 to display all plots, we can just tell them to use map(lambda x: x.show(), L) where L is the above list since (to me) this is not likely to be very desirable...

@vbraun
Copy link
Member Author

vbraun commented May 29, 2013

Changed dependencies from #14203 to #14203, #14266

@vbraun
Copy link
Member Author

vbraun commented May 29, 2013

comment:8

Rebased for

@vbraun
Copy link
Member Author

vbraun commented May 29, 2013

Changed dependencies from #14203, #14266 to #14203, #14266, #14471

@vbraun
Copy link
Member Author

vbraun commented May 30, 2013

comment:10

Changed patch to not catch any exceptions that might have come from generating the plot

@ppurka
Copy link
Member

ppurka commented May 31, 2013

comment:11

Just a heads up -- it is failing some doctests according to the patchbot.

@vbraun
Copy link
Member Author

vbraun commented May 31, 2013

Updated patch

@vbraun
Copy link
Member Author

vbraun commented May 31, 2013

comment:12

Attachment: trac_14469_repr_graphics.patch.gz

Should be fixed now.

@tscrim
Copy link
Collaborator

tscrim commented Jul 13, 2013

Reviewer: Travis Scrimshaw

@tscrim
Copy link
Collaborator

tscrim commented Jul 13, 2013

comment:13

Looks good to me.

@vbraun
Copy link
Member Author

vbraun commented Aug 6, 2013

Changed dependencies from #14203, #14266, #14471 to #14203, #14266, #14471, #15016

@vbraun
Copy link
Member Author

vbraun commented Aug 6, 2013

comment:15

This requires a version of the notebook that includes the commit vbraun/sagenb@89c6b6c. I've tentatively made a ticket for the upgrade to have something to put into the dependency fied.

@jdemeyer jdemeyer removed this from the sage-5.12 milestone Aug 7, 2013
@vbraun
Copy link
Member Author

vbraun commented Aug 8, 2013

comment:17

Fixed with the sagenb spkg from #15016

@vbraun vbraun added this to the sage-5.12 milestone Aug 8, 2013
@vbraun vbraun removed the pending label Aug 8, 2013
@jdemeyer
Copy link
Contributor

Merged: sage-5.12.beta1

@vbraun
Copy link
Member Author

vbraun commented Aug 20, 2013

comment:19

Followup at #15066

@novoselt
Copy link
Member

novoselt commented Sep 5, 2013

comment:20

I am not sure if this is the reason, but in a server-worker setup graphics is not shown in 5.12.beta4, while it does in 5.11 + sagenb-0.10.7.1.

@novoselt
Copy link
Member

novoselt commented Sep 6, 2013

comment:21

Made a ticket #15168 for this.

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

8 participants