Opened 9 years ago

Closed 9 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 jdemeyer)

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)

trac_10244_legend_label_fix.patch (1.1 KB) - added by mhampton 9 years ago.
Fixes problem by adding suboptions decorator to save command
trac_10244_legend_label_fix_v2.patch (1.7 KB) - added by mhampton 9 years ago.
trac_7981_and_10244_cumulative.patch (8.4 KB) - added by mhampton 9 years ago.
cumulative new patch for 7981 and 10244
trac_10244_legend_label_fix_v3.patch (1.7 KB) - added by kcrisman 9 years ago.
Rebased to #7981 and #8632
trac_10244-reviewer.patch (2.6 KB) - added by kcrisman 9 years ago.
reviewer patch

Download all attachments as: .zip

Change History (39)

comment:1 Changed 9 years ago by kcrisman

Thanks for noticing this, Marshall. In particular:

/Users/.../sage-4.6.1.alpha0/local/lib/python2.6/site-packages/sage/plot/plot.pyc in matplotlib(self, filename, xmin, xmax, ymin, ymax, figsize, figure, sub, axes, axes_labels, fontsize, frame, verify, aspect_ratio, gridlines, gridlinesstyle, vgridlinesstyle, hgridlinesstyle, show_legend, legend_options, axes_pad, ticks_integer, tick_formatter, ticks)
   1965             lopts.update(legend_options)
   1966             lopts.update(self.__legend_opts)
-> 1967             prop = FontProperties(family=lopts.pop('font_family'), weight=lopts.pop('font_weight'), \
   1968                     size=lopts.pop('font_size'), style=lopts.pop('font_style'), variant=lopts.pop('font_variant'))
   1969             color = lopts.pop('back_color')

So we've already popped everything while saving in .matplotlib, but then try to pop some other options like font_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.

comment:2 Changed 9 years ago by jason

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: Changed 9 years ago by 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.

comment:4 in reply to: ↑ 3 Changed 9 years ago by kcrisman

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 9 years ago by mhampton

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 9 years ago by mhampton

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 9 years ago by novoselt

  • Cc novoselt added

comment:8 Changed 9 years ago by mhampton

I think I figured this out, patch coming in a moment.

Changed 9 years ago by mhampton

Fixes problem by adding suboptions decorator to save command

comment:9 Changed 9 years ago by mhampton

  • Authors set to Marshall Hampton
  • Status changed from new to needs_review

comment:10 Changed 9 years ago by kcrisman

"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: Changed 9 years ago by kcrisman

  • Reviewers set to Karl-Dieter Crisman
  • Status changed from needs_review to 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 9 years ago by kcrisman

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 9 years ago by mhampton

comment:13 Changed 9 years ago by mhampton

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 9 years ago by mhampton

  • Status changed from needs_work to needs_review

comment:15 Changed 9 years ago by mhampton

  • Status changed from needs_review to needs_work

Just realized I need to format my test better for documentation.

comment:16 Changed 9 years ago by novoselt

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: Changed 9 years ago by 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.

comment:18 in reply to: ↑ 17 Changed 9 years ago by novoselt

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 9 years ago by mhampton

cumulative new patch for 7981 and 10244

comment:19 follow-up: Changed 9 years ago by mhampton

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 9 years ago by mhampton

  • Status changed from needs_work to needs_review

comment:21 in reply to: ↑ 19 Changed 9 years ago by kcrisman

Replying to mhampton:

OK, I think I fixed it so the reference manual looks OK.

This is fairly important, so I will try to review it soon.

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.

Agreed. See #10615.

comment:22 in reply to: ↑ 11 Changed 9 years ago by kcrisman

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 9 years ago by kcrisman

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 9 years ago by kcrisman

Rebased to #7981 and #8632

comment:24 Changed 9 years ago by kcrisman

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.

Changed 9 years ago by kcrisman

reviewer patch

comment:25 Changed 9 years ago by kcrisman

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:26 Changed 9 years ago by kcrisman

Just FYI - still applies fine on 4.6.2.alpha0.

comment:27 Changed 9 years ago by kcrisman

  • Status changed from needs_review to 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 9 years ago by novoselt

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 9 years ago by jdemeyer

  • Status changed from positive_review to needs_info

Please clarify which patches have to be applied.

comment:30 Changed 9 years ago by novoselt

  • Status changed from needs_info to needs_review

Please apply trac_10244_legend_label_fix_v3.patch and trac_10244-reviewer.patch

comment:31 Changed 9 years ago by novoselt

  • Status changed from needs_review to positive_review

comment:32 Changed 9 years ago by jdemeyer

  • Description modified (diff)

comment:33 Changed 9 years ago by novoselt

  • Milestone set to sage-4.6.2

comment:34 Changed 9 years ago by jdemeyer

  • Merged in set to sage-4.6.2.alpha2
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.