Opened 12 years ago
Closed 12 years ago
#10244 closed defect (fixed)
legends don't save correctly
Reported by: | mhampton | Owned by: | jason, was |
---|---|---|---|
Priority: | major | Milestone: | sage-4.6.2 |
Component: | graphics | Keywords: | |
Cc: | kcrisman, jason, novoselt | Merged in: | sage-4.6.2.alpha2 |
Authors: | Marshall Hampton | Reviewers: | Karl-Dieter Crisman |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
The new plot legends look great, but generate an error when you try to save them.
For example:
a1 = plot(sin,-pi,pi,legend_label='sin') a2 = plot(cos,-pi,pi,legend_label='cos',rgbcolor='red') two_plots = a1+a2 two_plots.save('./two_plots.png')
gives the error:
KeyError: 'pop(): dictionary is empty'
Apply: trac_10244_legend_label_fix_v3.patch and trac_10244-reviewer.patch
Attachments (5)
Change History (39)
comment:1 Changed 12 years ago by
comment:2 Changed 12 years ago by
Then a probably related issue is that this saving command:
plot(x,(x,0,1),aspect_ratio=1).save('test.png')
does *not* save a plot with aspect_ratio=1. If this isn't related, I guess we should open a new ticket.
comment:3 follow-up: 4 Changed 12 years ago by
comment:4 Changed 12 years ago by
Replying to jason:
I wonder if something like #7981 (or a rebased and updated version) could fix this? I think the problem in my comment is directly related to #7981.
Hmm, this seems quite likely - if the options are mostly used in show, not save. I'd have to actually try it out and follow the code through, though, and that won't happen soon :(
comment:5 Changed 12 years ago by
Unfortunately the current fix at #7981 does NOT fix this. Its extremely annoying for me, since I am currently in a project in which my group needs rather baroque plots and the legend is essential.
comment:6 Changed 12 years ago by
This seems like a clue:
sage: p= plot(x,(x,0,10),legend_label='Fantastic legend label', gridlines='major') sage: print p._extra_kwds {'gridlines': 'major'}
I would think that the legend info would show up in _extra_kwds, but I my understanding of the plotting code is weak.
comment:7 Changed 12 years ago by
Cc: | novoselt added |
---|
Changed 12 years ago by
Attachment: | trac_10244_legend_label_fix.patch added |
---|
Fixes problem by adding suboptions decorator to save command
comment:9 Changed 12 years ago by
Authors: | → Marshall Hampton |
---|---|
Status: | new → needs_review |
comment:10 Changed 12 years ago by
"maybe we never initialized them but then try to access them." Glad to know that I did sort of understand it :) If no one reviews this, I might look at it tomorrow.
comment:11 follow-up: 22 Changed 12 years ago by
Reviewers: | → Karl-Dieter Crisman |
---|---|
Status: | needs_review → needs_work |
This does fix the original problem. That should be doctested, something like
sage: P = plot(x,(x,0,1),legend_label='$xyz$') sage: P.set_legend_options(back_color=(1,0,0)) sage: P.set_legend_options(loc=7) sage: P.save() # but with the usual temp saving directory used!
It does NOT seem to fix Jason's issue with
plot(x,(x,0,1),aspect_ratio=1).save('test.png')
Nor does it fix this :(
sage: P = plot(x,(x,0,1),legend_label='$xyz$') sage: P.set_legend_options(back_color=(1,0,0)) sage: P # has nasty red background for label sage: Q = plot(x,(x,0,1),legend_label='$xyz$') sage: Q = plot(x,(x,0,1),legend_label='$xyz$',legend_back_color=(1,0,0)) --------------------------------------------------------------------------- RuntimeError Traceback (most recent call last) <snip> /Users/.../sage-4.6.1.rc1/local/lib/python2.6/site-packages/sage/plot/line.pyc in __init__(self, xdata, ydata, options) 46 for opt in options.iterkeys(): 47 if opt not in valid_options: ---> 48 raise RuntimeError("Error in line(): option '%s' not valid."%opt) 49 self.xdata = xdata 50 self.ydata = ydata RuntimeError: Error in line(): option 'legend_back_color' not valid.
So we aren't done yet. But probably that should be a different ticket, since these suboptions didn't work before anyway. Apparently we never doctested them! Which is probably my and/or Jason's fault... :(
comment:12 Changed 12 years ago by
Sorry - to be clear, the 'needs work' is with respect to the doctest which is needed.
Jason's thing is probably about SHOW_OPTIONS
having aspect_ratio
but save
not having it directly defined. Interestingly, in the documentation for save()
, this example DOES work.
EXAMPLES:: sage: c = circle((1,1),1,color='red') sage: filename=os.path.join(SAGE_TMP, 'test.png') sage: c.save(filename, xmin=-1,xmax=3,ymin=-1,ymax=3) To correct the aspect ratio of certain graphics, you can set the ``aspect_ratio`` to 1:: sage: c.save(filename, aspect_ratio=1, xmin=-1, xmax=3, ymin=-1, ymax=3)
But I think this is because we have new defaults for aspect_ratio
, not because this works in general.
sage: c = circle((1,1),1,color='red') sage: c.save(xmin=-1,xmax=5,aspect_ratio=4) # works sage: c = circle((1,1),1,color='red',aspect_ratio=4) sage: c.save(xmin=-1,xmax=5) # still aspect ratio 1
Any thoughts on how many tickets this should all get divided into?
Changed 12 years ago by
Attachment: | trac_10244_legend_label_fix_v2.patch added |
---|
comment:13 Changed 12 years ago by
Attached patch trac_10244_legend_label_fix_v2.patch is cumulative, includes Karl-Dieter's test suggestion.
I am also posting a cumulative patch with #7981 since they don't merge nicely.
comment:14 Changed 12 years ago by
Status: | needs_work → needs_review |
---|
comment:15 Changed 12 years ago by
Status: | needs_review → needs_work |
---|
Just realized I need to format my test better for documentation.
comment:16 Changed 12 years ago by
This is not directly related to this ticket, but are there any plans to make something better for the numerous plot options? My concern is that the default values seems to be repeated in different places, e.g. this ticket adds them for save. This makes is difficult to change the default values since one has to find all places where they are set. It would be nice also to provide some user interface for adjusting all defaults, i.e. let the user call some commands in the beginning of the session to affect all plots. I have tried to address this for fans and cones at #9664: single storage for all defaults and a possibility to tweak them. Clearly I like that approach since I have written it, but I think that it has some objective advantages.
comment:17 follow-up: 18 Changed 12 years ago by
That's a great idea. Some of these defaults can be manipulated already (such as xmin!) but I don't know that one would want to do each one by hand. If you have a concrete idea for making this all work (once tickets like these are closed) please put me in the cc: field. The plot code is already pretty hard for newcomers to find their way around, despite simplifications and refactorings of the past, so more help is great.
comment:18 Changed 12 years ago by
Replying to kcrisman:
That's a great idea. Some of these defaults can be manipulated already (such as xmin!) but I don't know that one would want to do each one by hand. If you have a concrete idea for making this all work (once tickets like these are closed) please put me in the cc: field. The plot code is already pretty hard for newcomers to find their way around, despite simplifications and refactorings of the past, so more help is great.
Well, in principal toric_plotter
(which is merged and is in the global name space) is concrete and working.
It has inside a dictionary for "Sage default options" and another one for "current user default options." Users can use commands like toric_plotter.options(ray_thickness=2)
to change defaults for consecutive commands (and I found it very convenient for use with SageTeX, where "Sage defaults" don't work nicely). There is no separation into options that are easy to change and options that are difficult to change, all are treated in the same way.
When a fan or a cone is plotted, it starts with a copy of "current user default options" and replaces any values in it with arguments of the plot command, if any. Then these options are used for actual plotting.
Also, all toric plotting commands have a reference to toric_plotter.options
so all options are described in a singe place, eliminating redundancy and improving consistency.
So I was thinking of something like plot.options(xmin=-10, xmax=10, color="red")
to affect all future plotting commands in the session. One of the drawbacks of current decorators like
sage: plot.options {'fillalpha': 0.5, 'detect_poles': False, 'plot_points': 200, 'thickness': 1, 'alpha': 1, 'adaptive_tolerance': 0.01, 'fillcolor': 'automatic', 'adaptive_recursion': 5, 'exclude': None, 'legend_label': None, 'rgbcolor': (0, 0, 1), 'fill': False}
is that there is no description of options and they are not always easy to understand, e.g. what is adaptive_recursion
? This is especially important if some options are "smartly" chosen based on others. E.g. it would be reasonable to set fill=True
if the user has explicitly provided fillcolor
, but such a behaviour should be explained. Note that in circle.options
there is FACEcolor
instead of FILLcolor
. I suppose they are the same but got named differently because of writing the same thing in different places. Inconsistency in naming parameters and their interpretation also makes it hard to write plotting code that works both for 2d and 3d, leading to extra if dim == 2: ... elif dim ==3: ...
for no good reason.
Unfortunately, I already put a bit on my plate so rewriting plotting code is not on my todo list for now and I just complain...
Changed 12 years ago by
Attachment: | trac_7981_and_10244_cumulative.patch added |
---|
cumulative new patch for 7981 and 10244
comment:19 follow-up: 21 Changed 12 years ago by
OK, I think I fixed it so the reference manual looks OK.
I agree these options are a mess, but cleaning it up like Andrey suggests is beyond the scope of this ticket and my available effort.
comment:20 Changed 12 years ago by
Status: | needs_work → needs_review |
---|
comment:21 Changed 12 years ago by
comment:22 Changed 12 years ago by
It does NOT seem to fix Jason's issue with
plot(x,(x,0,1),aspect_ratio=1).save('test.png')
This is fixed by #7981, thankfully.
Nor does it fix this :(
sage: Q = plot(x,(x,0,1),legend_label='$xyz$') sage: Q = plot(x,(x,0,1),legend_label='$xyz$',legend_back_color=(1,0,0)) --------------------------------------------------------------------------- RuntimeError Traceback (most recent call last) <snip> /Users/.../sage-4.6.1.rc1/local/lib/python2.6/site-packages/sage/plot/line.pyc in __init__(self, xdata, ydata, options) 46 for opt in options.iterkeys(): 47 if opt not in valid_options: ---> 48 raise RuntimeError("Error in line(): option '%s' not valid."%opt) 49 self.xdata = xdata 50 self.ydata = ydata RuntimeError: Error in line(): option 'legend_back_color' not valid.
This still isn't fixed in either patch, so it's now #10616.
comment:23 Changed 12 years ago by
This may need to be rebased for the slight change in #7981. Also would be best to have a separate patch for just this - if they don't merge nicely, maybe the original needs to be rebased?
Changed 12 years ago by
Attachment: | trac_10244_legend_label_fix_v3.patch added |
---|
comment:24 Changed 12 years ago by
Okay, this should apply correctly.
To buildbot: Depends on #7981 and #8632, apply only trac_10244_legend_label_fix_v3.patch
I noticed a few other things that apparently weren't caught the first time (meaning they were already present before this ticket), and may fix them in a brief reviewer patch shortly.
comment:25 Changed 12 years ago by
Turns out that some of the defaults weren't the same as the documentation. The legends look nice, so I changed the documentation to reflect the defaults. Also, it was handlelength
, not handlelen
. I also accidentally didn't get the right formatting since I based it on the earlier patch, so I fixed that goof of mine in the rebase.
This is significant enough that I would appreciate a review by someone of the review patch - original author is fine! But all should be well.
To buildbot: Depends on #7981 and #8632, apply trac_10244_legend_label_fix_v3.patch and trac_10244-reviewer.patch
comment:27 Changed 12 years ago by
Status: | needs_review → positive_review |
---|
Upon actually looking at the patch I generated, it's much smaller than I thought, and only a few numbers are changed in the doc (the other change is fixing the formatting, which Marshall had already done). So I'm comfortable with giving this positive review after all.
comment:28 Changed 12 years ago by
Depends on #8632
(I think that build bot get's confused if we methion the first one which is already listed in the second one).
comment:29 Changed 12 years ago by
Status: | positive_review → needs_info |
---|
Please clarify which patches have to be applied.
comment:30 Changed 12 years ago by
Status: | needs_info → needs_review |
---|
Please apply trac_10244_legend_label_fix_v3.patch and trac_10244-reviewer.patch
comment:31 Changed 12 years ago by
Status: | needs_review → positive_review |
---|
comment:32 Changed 12 years ago by
Description: | modified (diff) |
---|
comment:33 Changed 12 years ago by
Milestone: | → sage-4.6.2 |
---|
comment:34 Changed 12 years ago by
Merged in: | → sage-4.6.2.alpha2 |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
Thanks for noticing this, Marshall. In particular:
So we've already popped everything while saving in
.matplotlib
, but then try to pop some other options likefont_family
. I would think those were already given in some@options
thingie, but maybe they weren't. I don't have time to check more into this, but I suspect the suboptions decorator has something to do with this - maybe we never initialized them but then try to access them.