legends don't save correctly

[5d2aaf09-c963-473a-bf79-1f742a72700f]

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: attachment: trac_10244_legend_label_fix_v3.patch and attachment: trac_10244-reviewer.patch

CC: @kcrisman @jasongrout @novoselt

Component: graphics

Author: Marshall Hampton

Reviewer: Karl-Dieter Crisman

Merged: sage-4.6.2.alpha2

Issue created by migration from https://trac.sagemath.org/ticket/10244

[kcrisman]

comment:1

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.

[jasongrout]

comment:2

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.

[jasongrout]

comment:3

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.

[kcrisman]

comment:4

Replying to @jasongrout:

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 :(

[5d2aaf09-c963-473a-bf79-1f742a72700f]

comment:5

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.

[5d2aaf09-c963-473a-bf79-1f742a72700f]

comment:6

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.

[5d2aaf09-c963-473a-bf79-1f742a72700f]

comment:8

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

[5d2aaf09-c963-473a-bf79-1f742a72700f]

Fixes problem by adding suboptions decorator to save command

[5d2aaf09-c963-473a-bf79-1f742a72700f]

Author: Marshall Hampton

[5d2aaf09-c963-473a-bf79-1f742a72700f]

comment:9

Attachment: trac_10244_legend_label_fix.patch.gz

[kcrisman]

comment:10

"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.

[kcrisman]

comment:11

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... :(

[kcrisman]

Reviewer: Karl-Dieter Crisman

[kcrisman]

comment:12

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?

[5d2aaf09-c963-473a-bf79-1f742a72700f]

comment:13

Attachment: trac_10244_legend_label_fix_v2.patch.gz

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.

[5d2aaf09-c963-473a-bf79-1f742a72700f]

comment:15

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

[novoselt]

comment:16

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.

[kcrisman]

comment:17

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.

[novoselt]

comment:18

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...

[5d2aaf09-c963-473a-bf79-1f742a72700f]

cumulative new patch for 7981 and 10244

[5d2aaf09-c963-473a-bf79-1f742a72700f]

comment:19

Attachment: trac_7981_and_10244_cumulative.patch.gz

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.

[kcrisman]

comment:21

Replying to @sagetrac-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.

[kcrisman]

comment:22

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.

[kcrisman]

comment:23

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?

[kcrisman]

Rebased to #7981 and #8632

[kcrisman]

comment:24

Attachment: trac_10244_legend_label_fix_v3.patch.gz

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.

[kcrisman]

reviewer patch

[kcrisman]

comment:25

Attachment: trac_10244-reviewer.patch.gz

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

[kcrisman]

comment:26

Just FYI - still applies fine on 4.6.2.alpha0.

[kcrisman]

comment:27

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.

[novoselt]

comment:28

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).

[jdemeyer]

comment:29

Please clarify which patches have to be applied.

[novoselt]

comment:30

Please apply trac_10244_legend_label_fix_v3.patch and trac_10244-reviewer.patch

[jdemeyer]

Description changed:

--- 
+++ 
@@ -15,3 +15,4 @@
 KeyError: 'pop(): dictionary is empty'

+Apply: attachment: trac_10244_legend_label_fix_v3.patch and attachment: trac_10244-reviewer.patch

[jdemeyer]

Merged: sage-4.6.2.alpha2