#13732 closed defect (fixed)
Fix alpha in disk() graphics object so that we can save PDF's
Reported by: | john_perry | Owned by: | jason, was |
---|---|---|---|
Priority: | trivial | Milestone: | sage-5.8 |
Component: | graphics | Keywords: | PDF, matplotlib, save, alpha |
Cc: | Merged in: | sage-5.8.beta0 | |
Authors: | John Perry | Reviewers: | Karl-Dieter Crisman |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
We want to fix the following error on sage 5.4. Given the input
sage: p = disk((0,0), 5, (0, pi/4), color='red') sage: p += disk((0,0), 5, (pi/4, pi/2), color='red', alpha=0.5) sage: p.save("test.pdf")
we get the error
TypeError: Don't know a PDF representation for <type 'sage.rings.real_mpfr.RealLiteral'> objects.
This can be fixed in the backend quite easily.
Note: The problem appears to occur only in disk()
. In fact, it's clear that someone manually fixed this for other objects earlier, or else programmed it correctly and overlooked disk()
. The patch fixes the problem in a way that is consistent with the other approaches.
Apply:
Attachments (1)
Change History (17)
comment:1 Changed 6 years ago by
- Status changed from new to needs_review
comment:2 follow-up: ↓ 3 Changed 6 years ago by
Hi, nice start! Very comprehensive. Here are some comments, some of which make it "needs work".
- I think the preferred syntax is
if alpha is not None
.In [12]: % timeit D is not None 10000000 loops, best of 3: 43.8 ns per loop In [13]: % timeit D != None 10000000 loops, best of 3: 65.1 ns per loop
- Note that at least sometimes alpha does work. Let's only fix the ones that do (see #14074 for that polygons apparently work). Similarly for the doctests of it, naturally.
- You cannot just save
p.save("test.pdf")
, unfortunately, as that will produce files left over after doctesting. There is a framework for temporary files - in fact, several competing ones - for doctesting, there should be some good examples in thesave
method (?). - When referencing Trac tickets, you might as well put it in the main line, e.g.
Save alpha information in pdf (see :trac:`13732`)::
comment:3 in reply to: ↑ 2 Changed 6 years ago by
Replying to kcrisman:
Hi, nice start! Very comprehensive.
Thanks :-)
- I think the preferred syntax is
if alpha is not None
.In [12]: % timeit D is not None 10000000 loops, best of 3: 43.8 ns per loop In [13]: % timeit D != None 10000000 loops, best of 3: 65.1 ns per loop
Oh, wow. I'll take care of that.
- Note that at least sometimes alpha does work. Let's only fix the ones that do (see #14074 for that polygons apparently work). Similarly for the doctests of it, naturally.
Ack. I'd had so many problems with alpha that I just assumed saving it wouldn't work on anything. Still, that makes sense, considering that coordinates don't raise objections from matplotlib.
- You cannot just save
p.save("test.pdf")
, unfortunately, as that will produce files left over after doctesting. There is a framework for temporary files - in fact, several competing ones - for doctesting, there should be some good examples in thesave
method (?).
Alright; I'll look for that.
- When referencing Trac tickets, you might as well put it in the main line, e.g.
Save alpha information in pdf (see :trac:`13732`)::
I thought there was something, but I couldn't remember it, and I figured that someone would correct me if so. Bingo. :-D
Okay, I'll get to work fixing this now.
comment:4 follow-up: ↓ 5 Changed 6 years ago by
- Description modified (diff)
NEEDS REVIEW
It looks like you forgot to change the status to "needs work", so I will leave it as "needs review" ;-)
It turns out that all the other files I had worked on were okay. That gave me the idea to look at how arc()
handles the same problems, since it also accepts the alpha
keyword. By mimicking that solution (which also showed me why the coordinates don't raise the same error), I avoided the whole != None
problem.
To be honest, it looks as if someone either noticed this problem before & tried to fix it, remembering everything except disk()
. Here I was thinking I was making a great contribution, when in fact I was just cleaning up behind someone else. I'll go weep into some hot tea. ;-)
By the way, I hope you're not too snowed in, way up there in New England.
comment:5 in reply to: ↑ 4 ; follow-up: ↓ 6 Changed 6 years ago by
- Reviewers set to Karl-Dieter Crisman
It turns out that all the other files I had worked on were okay. That gave me the idea to look at how
arc()
handles the same problems, since it also accepts thealpha
keyword. By mimicking that solution (which also showed me why the coordinates don't raise the same error), I avoided the whole!= None
problem.
Excellent. I'm sure that I've seen all this before, too, and didn't recognize it. That's the problem with viewing patches and not the whole code.
I suppose one could run into trouble anyway, if one used the Disk
primitive and called this method, but that's not intended use and it's pervasive in the code, so it would be another ticket, if even worth opening.
You may want to fix the commit line.
Track 13732: fix output of real data in matplotlib PDF backend
but otherwise positive review.
To be honest, it looks as if someone either noticed this problem before & tried to fix it, remembering everything except
disk()
. Here I was thinking I was making a great contribution, when in fact I was just cleaning up behind someone else. I'll go weep into some hot tea. ;-)
Hey, it got fixed, right? :-)
By the way, I hope you're not too snowed in, way up there in New England.
Oh yes. Took over two hours to shovel all the way out to where the plows "created" the street so the car is ready to get out - though now church is cancelled due to lack of parking, so I guess tomorrow is another day at home.
comment:6 in reply to: ↑ 5 Changed 6 years ago by
- Description modified (diff)
- Summary changed from Fix output of real data in matplotlib PDF backend to Fix alpha in disk() graphics object so that we can save PDF's
Replying to kcrisman:
You may want to fix the commit line.
Track 13732: fix output of real data in matplotlib PDF backendbut otherwise positive review.
I've fixed that commit message now (& I'll also change the ticket description to match the problem more accurately).
By the way, I hope you're not too snowed in, way up there in New England.
Oh yes. Took over two hours to shovel all the way out to where the plows "created" the street so the car is ready to get out - though now church is cancelled due to lack of parking, so I guess tomorrow is another day at home.
I don't doubt how that will be a disappointment for you. In fact, Psalm 42 may be on my mind tomorrow morning, but in my case I'm getting over a mysterious bronchitis.
comment:7 follow-up: ↓ 8 Changed 6 years ago by
- Status changed from needs_review to positive_review
Ok, looks good.
Hope things are okay in Hattiesburg - just read in the paper that a tornado came through?
comment:8 in reply to: ↑ 7 ; follow-ups: ↓ 9 ↓ 11 Changed 6 years ago by
Replying to kcrisman:
Hope things are okay in Hattiesburg - just read in the paper that a tornado came through?
Only in the paper? You must not watch CNN! :-)
They're saying it was an EF-3. The southern edge of campus was ripped up but good: several buildings were damaged, one severely, and one apparently destroyed. So far, there are no reports of serious injuries, which strikes me as a miracle.
comment:9 in reply to: ↑ 8 ; follow-up: ↓ 10 Changed 6 years ago by
Hope things are okay in Hattiesburg - just read in the paper that a tornado came through?
Only in the paper? You must not watch CNN! :-)
Correct - no cable, and most TV watched is fiction or kid stuff.
They're saying it was an EF-3. The southern edge of campus was ripped up but good: several buildings were damaged, one severely, and one apparently destroyed. So far, there are no reports of serious injuries, which strikes me as a miracle.
Indeed. A similar story at a sister school of ours in Jackson, TN a few years back.
comment:10 in reply to: ↑ 9 Changed 6 years ago by
Replying to kcrisman:
Correct - no cable, and most TV watched is fiction or kid stuff.
A distinction without a difference, if you ask me. ;-)
comment:11 in reply to: ↑ 8 Changed 6 years ago by
Replying to john_perry:
They're saying it was an EF-3.
Correction: according to tonight's (local) news, F-4.
comment:12 Changed 6 years ago by
- Milestone changed from sage-5.7 to sage-5.8
comment:13 Changed 6 years ago by
- Merged in set to sage-5.8.beta0
- Resolution set to fixed
- Status changed from positive_review to closed
comment:14 follow-up: ↓ 15 Changed 6 years ago by
I get the same error in Sage 5.9 with:
sage: f = lambda x, y : (abs(cos((x + I * y) ** 4)) - 1) sage: g2 = implicit_plot(f,(-4, 4),(-3, 3),linewidth=0.6) sage: g2.save('/tmp/cosz4_2.pdf') TypeError: Don't know a PDF representation for <type 'sage.rings.real_mpfr.RealLiteral'> objects.
Replacing 0.6
by float(0.6)
solves the problem. Should I reopen or open a new ticket?
Paul
comment:15 in reply to: ↑ 14 Changed 6 years ago by
Replying to zimmerma:
TypeError?: Don't know a PDF representation for <type 'sage.rings.real_mpfr.RealLiteral?'> objects. }}} Replacing
0.6
byfloat(0.6)
solves the problem. Should I reopen or open a new ticket?
I think it needs a new ticket, for two reasons.
- It has to do with coordinates, not
alpha
. You might want to see ifalpha
works withimplicit_plot()
, but... - The fix given here for
disk()
really does fixdisk()
.
comment:16 Changed 6 years ago by
I think it needs a new ticket
see #14741
Fixing it in the backend was easy, though the fix was strongly discommended, so I applied a different fix, which was fairly easy. I added a very simple doctest to each modified file.
I have a feeling there's a better way to do this; after all, the same bug doesn't show up when decimals are used for coordinates, etc. However, this works for now.