Opened 9 years ago

Closed 8 years ago

Last modified 8 years ago

#14469 closed defect (fixed)

Fix _repr_ of graphics objects

Reported by: vbraun Owned by: jason, was
Priority: major Milestone: sage-5.12
Component: graphics Keywords:
Cc: ppurka Merged in: sage-5.12.beta1
Authors: Volker Braun Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #14203, #14266, #14471, #15016 Stopgaps:

Status badges

Description (last modified by vbraun)

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?

Attachments (1)

trac_14469_repr_graphics.patch (24.8 KB) - added by vbraun 9 years ago.
Updated patch

Download all attachments as: .zip

Change History (22)

comment:1 Changed 9 years ago by vbraun

  • Description modified (diff)

comment:2 Changed 9 years ago by vbraun

  • Authors set to Volker Braun
  • Dependencies set to #14203
  • Status changed from new to needs_review

comment:3 Changed 9 years ago by vbraun

  • Status changed from needs_review to needs_work

comment:4 Changed 9 years ago by ppurka

  • Cc ppurka added

comment:5 Changed 9 years ago by kcrisman

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.

comment:6 Changed 9 years ago by vbraun

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

comment:7 Changed 9 years ago by tscrim

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

comment:8 Changed 9 years ago by vbraun

  • Dependencies changed from #14203 to #14203, #14266

Rebased for

comment:9 Changed 9 years ago by vbraun

  • Dependencies changed from #14203, #14266 to #14203, #14266, #14471
  • Status changed from needs_work to needs_review

comment:10 Changed 9 years ago by vbraun

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

comment:11 Changed 9 years ago by ppurka

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

Changed 9 years ago by vbraun

Updated patch

comment:12 Changed 9 years ago by vbraun

Should be fixed now.

comment:13 Changed 8 years ago by tscrim

  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

Looks good to me.

comment:14 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:15 Changed 8 years ago by vbraun

  • Dependencies changed from #14203, #14266, #14471 to #14203, #14266, #14471, #15016

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

comment:16 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-5.12 to sage-pending

comment:17 Changed 8 years ago by vbraun

  • Milestone changed from sage-pending to sage-5.12

Fixed with the sagenb spkg from #15016

comment:18 Changed 8 years ago by jdemeyer

  • Merged in set to sage-5.12.beta1
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:19 Changed 8 years ago by vbraun

Followup at #15066

comment:20 Changed 8 years ago by novoselt

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.

comment:21 Changed 8 years ago by novoselt

Made a ticket #15168 for this.

Note: See TracTickets for help on using tickets.