Opened 7 years ago

Closed 6 years ago

Last modified 6 years ago

#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 john_perry)

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:

  1. trac_13732.patch

Attachments (1)

trac_13732.patch (1.2 KB) - added by john_perry 6 years ago.
patches matplotlib backend so that RealLiteral? has an output

Download all attachments as: .zip

Change History (17)

comment:1 Changed 6 years ago by john_perry

  • Status changed from new to needs_review

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.

comment:2 follow-up: Changed 6 years ago by kcrisman

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 the save 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 john_perry

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 the save 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: Changed 6 years ago by john_perry

  • 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: Changed 6 years ago by kcrisman

  • Authors set to John Perry
  • 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 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.

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.

Changed 6 years ago by john_perry

patches matplotlib backend so that RealLiteral? has an output

comment:6 in reply to: ↑ 5 Changed 6 years ago by john_perry

  • 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 backend

but 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: Changed 6 years ago by kcrisman

  • 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: Changed 6 years ago by john_perry

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: Changed 6 years ago by 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! :-)

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 john_perry

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 john_perry

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 jdemeyer

  • Milestone changed from sage-5.7 to sage-5.8

comment:13 Changed 6 years ago by jdemeyer

  • Merged in set to sage-5.8.beta0
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:14 follow-up: Changed 6 years ago by zimmerma

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 john_perry

Replying to zimmerma:

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?

I think it needs a new ticket, for two reasons.

  1. It has to do with coordinates, not alpha. You might want to see if alpha works with implicit_plot(), but...
  2. The fix given here for disk() really does fix disk().

comment:16 Changed 6 years ago by zimmerma

I think it needs a new ticket

see #14741

Note: See TracTickets for help on using tickets.