#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: |
Description (last modified by )
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)
Change History (22)
comment:1 Changed 9 years ago by
- Description modified (diff)
comment:2 Changed 9 years ago by
- Dependencies set to #14203
- Status changed from new to needs_review
comment:3 Changed 9 years ago by
- Status changed from needs_review to needs_work
comment:4 Changed 9 years ago by
- Cc ppurka added
comment:5 Changed 9 years ago by
comment:6 Changed 9 years ago by
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
Two things on the current patch:
- Is there a reason why you're importing
DOCTEST_MODE
indisplayhook.py
? You don't seem to be using it. - The method
_graphics_()
insage_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:9 Changed 9 years ago by
- Dependencies changed from #14203, #14266 to #14203, #14266, #14471
- Status changed from needs_work to needs_review
comment:10 Changed 9 years ago by
Changed patch to not catch any exceptions that might have come from generating the plot
comment:11 Changed 9 years ago by
Just a heads up -- it is failing some doctests according to the patchbot.
comment:12 Changed 9 years ago by
Should be fixed now.
comment:13 Changed 8 years ago by
- Reviewers set to Travis Scrimshaw
- Status changed from needs_review to positive_review
Looks good to me.
comment:14 Changed 8 years ago by
- Milestone changed from sage-5.11 to sage-5.12
comment:15 Changed 8 years ago by
- 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
- Milestone changed from sage-5.12 to sage-pending
comment:17 Changed 8 years ago by
- Milestone changed from sage-pending to sage-5.12
Fixed with the sagenb spkg from #15016
comment:18 Changed 8 years ago by
- 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
Followup at #15066
comment:20 Changed 8 years ago by
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
Made a ticket #15168 for this.
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.