aspect_ratio and figsize for graphics do not work as expected in sage-4.8.alpha5

williamstein commented 12 years ago

I don't know what was done to plotting in sage-4.8.alpha5, but it is seriously messed up. Basically, something is broken so that figsize doesn't work at all. Often I have to set an aspect ratio of 1000 just to see anything, etc. Here's a simple example, in which both outputs looks fine in sage-4.7.2, but in sage-4.8.alpha5, it is a total disaster:

g = Graphics(); g += line([(0, 120.93), (1, 120.92)])
g.show()
g.show(figsize=[8,4])

To see this on a remote install, just use "save('a.png')" in place of show.

Even though the first show is broken, perhaps due to something being set by default, the g.show(figsize=[8,4]) is supposed to produce an "8 inch by 4 inch figure", but instead it makes something that is 0 inches tall. What's going on? What broke stuff?

Component: graphics

Author: William Stein

Reviewer: Jason Grout

Merged: sage-4.8.alpha5

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

williamstein commented 12 years ago

comment:1

This bug is caused by #11963.

kcrisman commented 12 years ago

comment:2

This bug is caused by #11963.

Maybe it could be reverted and that one put to 'needs work'?

kcrisman commented 12 years ago

comment:3

Okay, the problem is that Graphics() continues to have default

return self._extra_kwds.get('aspect_ratio',1.0)

but since we now actually do what we advertised before (namely, even in the old code we had that a non-'automatic' aspect ratio was to be trumped by a numerical value) this has cropped up.

Old:

if self.__aspect_ratio=='automatic': 
     g.__aspect_ratio=other.__aspect_ratio 
elif other.__aspect_ratio=='automatic': 
     g.__aspect_ratio=self.__aspect_ratio 
else: 
     g.__aspect_ratio = max(self.__aspect_ratio, other.__aspect_ratio)

so not really anything has changed, we just actually do it now, and this exposed that. Even before we had

self.__aspect_ratio = 1.0

for empty graphics objects. And we do want to keep it at that for everything except things like point, line, plot. Maybe can Graphics() just have an empty aspect ratio to handle this situation, and update the if/elif/else clause accordingly?

novoselt commented 12 years ago

comment:4

One more example to consider while we are fixing this:

g = circle((0, 120.93), 0.0001)
g += circle((1, 120.92), 0.0001)
g.show()
g.show(figsize=[8,4])

In 4.7.2 both plots are quite bad with a bunch of vertical labels on top of each other. As far as I understand, the last command is NOT supposed to produce a plot 8x4 inches, but rather a plot with the current aspect ratio with one of the dimensions equal to given ones and the other one less than or equal. So this behaviour is correct, with aspect ratio being 1 for each circle, but perhaps it is worth to distinguish "automatic 1" and "user set 1". If the user really wants to have a long plot in 1:1, that's what should be done, but otherwise "round circle considerations" should be dropped when the ratio of dimensions reaches, say, 5 or 1/5.

williamstein commented 12 years ago

Attachment: trac_12213.patch.gz

williamstein commented 12 years ago

comment:5

I've attached a 1-line patch that fixes everything as far as I'm concerned. I think it makes a lot more sense as well. All I did was make it so the default for aspect_ratio is now 'automatic'... which kind of makes sense right, since 1.0 is almost never what one actually wants.

As long as you don't explicitly set the aspect_ratio (or add in the result of plot, which sets aspect_ratio), then the best automatic default is chosen, probably by Matplotlib; moreover, figsize=[w,h] really does produce a figure that has the right width and height.

With this patch, both my example and novoselt's look fine.

williamstein commented 12 years ago

comment:6

Replying to @novoselt:

As far as I understand, the last command is NOT supposed to produce a plot 8x4 inches, but rather a plot with the current aspect ratio with one of the dimensions equal to given ones and the other one less than or equal.

Why? I can't find any documentation anywhere that says this. It also seems very weird to me. If I do:

sage: G = ...
sage: G.show(figsize=[8,4])

I obviously want a figure that is 8 x 4. If I merely wanted to expand one dimension keeing the aspect ratio, I would do

sage: G = ...
sage: G.show(figsize=8)

That said, I think this is an issue for another ticket.

kcrisman commented 12 years ago

comment:7

I've attached a 1-line patch that fixes everything as far as I'm concerned. I think it makes a lot more sense as well. All I did was make it so the default for aspect_ratio is now 'automatic'... which kind of makes sense right, since 1.0 is almost never what one actually wants.

Did you check whether contour/parametric plots, circles, other primitives still behave "as advertised" with this? That is the major case where aspect ratio needs to be 1. In fact, for most of the code in plot/, that's what we wanted; it was only (the very heavily used, granted!) things that weren't primitives of that kind that we changed with the options decorator to have automatic.

If I Recall Correctly, which I often don't :( Anyway, it's worth opening a "live" copy of the Sage reference manual and just doing 'evaluate all' on the plot pages to make sure all is well.

novoselt commented 12 years ago

comment:8

I think that the aspect ratio for circles was deliberately set to 1 in #2100 and from glancing at its comments it seems that it was intentionaly set to 1 for almost everything. Which I actually find quite nice for many plots, but of course not for the ones on this ticket.

In any case - if we do the change in William's patch, there may be some pieces of documentation that have to be updated.

novoselt commented 12 years ago

comment:9

Replying to @williamstein:

Replying to @novoselt:

As far as I understand, the last command is NOT supposed to produce a plot 8x4 inches, but rather a plot with the current aspect ratio with one of the dimensions equal to given ones and the other one less than or equal.

Why? I can't find any documentation anywhere that says this. It also seems very weird to me. If I do:

sage: G = ...
sage: G.show(figsize=[8,4])

I obviously want a figure that is 8 x 4.

I would argue that I want to have a graphics object of dimensions 8 x 4, i.e. the canvas should be of this dimension. But inside it I would expect to have the plot with the set aspect ratio, if it was actually set.

If I merely wanted to expand one dimension keeing the aspect ratio, I would do

sage: G = ...
sage: G.show(figsize=8)

That said, I think this is an issue for another ticket.

It seems to me that this is the same as requesting 8 x 8 figure. My point was that show with figsize should not behave very different from show, i.e. it should not override the aspect ratio.

kcrisman commented 12 years ago

comment:10

I think that the aspect ratio for circles was deliberately set to 1 in #2100 and from glancing at its comments it seems that it was intentionaly set to 1 for almost everything. Which I actually find quite nice for many plots, but of course not for the ones on this ticket.

They aren't supposed to be 1 for was's problem; what happened is that Graphics() had a default aspect ratio which in the past we didn't see when adding.

I'm not sure what to do about novoselt's example. Clearly we don't want a.r.=1, but I also don't think we should just have circles have the "wrong" aspect ratio on their own.

I guess one could try to have something that looked at the bounding boxes once we added two primitives and checked if the dimensions in the current aspect ratio were worse than some "horrible" ones, and then set to 'automatic' or something. But I think that will both slow things down for adding a lot of plots (which is quite typical) and also just give rise to other problems. In the end, sometimes the user just has to set the aspect ratio; I don't think there is a uniform way around that without getting lots of circles that look like ellipses by default.

williamstein commented 12 years ago

comment:11

Replying to @novoselt:

Replying to @williamstein:

Replying to @novoselt:

As far as I understand, the last command is NOT supposed to produce a plot 8x4 inches, but rather a plot with the current aspect ratio with one of the dimensions equal to given ones and the other one less than or equal.

Why? I can't find any documentation anywhere that says this. It also seems very weird to me. If I do:
sage: G = ...
sage: G.show(figsize=[8,4])
I obviously want a figure that is 8 x 4.

I would argue that I want to have a graphics object of dimensions 8 x 4, i.e. the canvas should be of this dimension. But inside it I . would expect to have the plot with the set aspect ratio, if it was actually set.

What you describe is (1) not what I expect, (2) not what is documented (if anything), and (3) not what actually happens in Sage now (or any previous version). I don't understand what use it would be to have that behavior either.

If I merely wanted to expand one dimension keeing the aspect ratio, I would do
sage: G = ...
sage: G.show(figsize=8)
That said, I think this is an issue for another ticket.

It seems to me that this is the same as requesting 8 x 8 figure.

No it isn't. If you want an 8x8 figure you would do figsize=[8,8].

My point was that show with figsize should not behave very different from show, i.e. it should not override the aspect ratio.

I disagree. If one explicitly specifies a figsize, then I think it is should override aspect_ratio, because a figsize with two inputs implies an aspect ratio.

williamstein commented 12 years ago

comment:12

Replying to @kcrisman:

I've attached a 1-line patch that fixes everything as far as I'm concerned. I think it makes a lot more sense as well. All I did was make it so the default for aspect_ratio is now 'automatic'... which kind of makes sense right, since 1.0 is almost never what one actually wants.

Did you check whether contour/parametric plots, circles, other primitives still behave "as advertised" with this?

I can't find anywhere in plot.py that circles are advertised as having aspect_ratio 1.

In the 3d plotting code, spheres get aspect [1:1:1] by default, and I think everything else gets something automatic. So once a plot contains a sphere, it inherits ratio 1, unless something with a different ratio gets added. For consistency, we could do the same with circles, i.e., they get aspect 1, but everything else gets automatic.

I'm not sure about parametric and contour plots.

The only thing I really want is for Graphics() to have aspect ratio "automatic" by default.

kcrisman commented 12 years ago

comment:13

Replying to @williamstein:

Replying to @kcrisman:

I've attached a 1-line patch that fixes everything as far as I'm concerned. I think it makes a lot more sense as well. All I did was make it so the default for aspect_ratio is now 'automatic'... which kind of makes sense right, since 1.0 is almost never what one actually wants.

Did you check whether contour/parametric plots, circles, other primitives still behave "as advertised" with this?

I can't find anywhere in plot.py that circles are advertised as having aspect_ratio 1.

Conspicuous by its absence! See circle, for instance. The so-called example for aspect ratio and figsize is not necessary, as far as I can tell, in addition to having mistakenly been placed under the 3d plotting example where it doesn't belong!

Interestingly, we forgot to fix the top of the main plot doc in #2100. Note also that the very first example shows that, in 4.7.2, we still have the previous addition happening, even though it isn't supposed to, so that one was a symptom of #11963.

Anyway, we can have that default, but then we'll need to make sure that all those primitives like circles are @optioned to have aspect ratio 1. The discussion at #2100 is definitely worth reading on that score, it was reasonably comprehensive.

novoselt commented 12 years ago

comment:14

My comment about circles was after looking at William's patch: it shows that circles stop being round. I also didn't see comments on round circles in 4.7.2 documentation, but I remember reading some documentation patches where it was explained that they are round by default. Unfortunately, I don't recall where it was.

My use case for figsize not changing aspect ratio (unrelated to the final canvas size):

I have a plot where I set aspect ratio to something I want
now I want to insert this picture into a paper/presentation, e.g. using SageTeX
I give dimensions of the picture place through figsize and get the size which I want with the aspect ratio which I want

Scaling the plot using TeX options is not a good idea: even in vector formats it will mess up e.g. font sizes (and that's one of the things I really like about SageTeX: plot labels can coordinate in size with the surrounding text).

Computing precise dimensions of the plot and figuring out what should be the figsize to fit where I want with the aspect ratio that I want is not cool.

Giving a single parameter to figsize to change only width is nice, but sometimes I want to make a particular height, not width. So keeping the ratio and expanding the plot until it hits one of the bounds is convenient...

jasongrout commented 12 years ago

comment:16

Wow, this is quite the discussion! First, let me point out the documentation for the fig_tight show keyword, documented in save(): http://www.sagemath.org/doc/reference/sage/plot/plot.html#sage.plot.plot.Graphics.save and show(): http://www.sagemath.org/doc/reference/sage/plot/plot.html#sage.plot.plot.Graphics.show. There should probably also be a mention of this at the top of the file in the examples dealing with figsize. Basically, by default, figsize specifies an upper bound, and the resulting image is cropped to what the actual image is. If fig_tight=False (i.e., don't crop tightly to the figure), then the resulting image should be exactly the size you specify.

William's solution here (default aspect ratio of 'automatic' in Graphics() ) is what I was thinking might be the best idea when I was reading the initial discussion on sage-devel. I can't think of any bad consequences of this, though I haven't had time to test it.

williamstein commented 12 years ago

comment:17

I don't feel like I really understand fig_tight:

Where did this fig_tight option come from? I co-wrote the plotting code back in 2006-2007, and there was no fig_tight option. I'm guessing somebody (probably Jason) added it to expose some functionality in matplotlib? What was the motivation?
Why is the default True instead of False?

A search found #2100 which is very relevant. It documents fig_tight in a way that is understandable (probably better than Sage documentation pointed to above). It also has a discussion about aspect_ratio and how automatic is supposed to be the default. So I think this proves definitively that #11963 is wrong and needs to be fixed.

That said, I still don't understand why when I write g.show(figsize=[w,h]) I should ever get a figure that isn't w "inches" wide and h inches tall? I simply don't understand why an option like fig_tight (which involves cropping) is even relevant.

williamstein commented 12 years ago

comment:18

Replying to @novoselt:

My use case for figsize not changing aspect ratio (unrelated to the final canvas size): [...] Giving a single parameter to figsize to change only width is nice, but sometimes I want to make a particular height, not width. So keeping the ratio and expanding the plot until it hits one of the bounds is convenient...

I'm definitely not against having the functionality you want. I just don't think it makes sense given what figsize is. For example, we could make it so one could do

   figsize=[None, 8]
and
   figsize=[12, None]

and the None gets replaced by the maximum that fits.

jasongrout commented 12 years ago

comment:19

You're right that the fig_tight option exposes extra matplotlib functionality that we didn't have before. The idea was to avoid large expanses of whitespace in an image when the aspect ratio and the figure size didn't quite match up. The default (fig_tight=True) is to give nicely cropped pictures, with the figsize as a guide to how big the picture should be. When fig_tight is true, think of the figsize as being the size of the canvas, then we draw the picture on it, then we crop the canvas to what we drew to get a nice image without excessive borders. The problem with it being False by default is that we end up with most figures having excessive whitespace in the borders (because they have a default figsize that probably does not match their aspect ratio).

It seems reasonable that if figsize is explicitly set for both dimensions, the fig_tight cropping shouldn't happen by default. I think if figsize is not specified (i.e., is the default, I think 6 in by 4 in), we should definitely have fig_tight = True by default. So: is that policy too confusing?

jasongrout commented 12 years ago

comment:20

to clarify: is it too confusing that cropping happens in one case, but not in the other? I think probably not; that it is better to have fig_tight default to True if figsize is not explicitly set (in both dimensions), but default to False if figsize is explicitly set in both dimensions.

jasongrout commented 12 years ago

comment:21

Applies cleanly to 4.8.alpha4, passes tests in plot/*.py[x], and is a very reasonable default, I think. Positive review.

jdemeyer commented 12 years ago

Reviewer: Jason Grout

jdemeyer commented 12 years ago

Author: William Stein

jdemeyer commented 12 years ago

Merged: sage-4.8.alpha5

jdemeyer commented 12 years ago

comment:23

I hope this has been doctested properly...

kcrisman commented 12 years ago

comment:24

I hope this has been doctested properly...

It hasn't. We should have at least two or three of the examples above in the documentation under TESTS so that future people can easily check for regression.

I am also worried about whether anyone bothered to check all the many pages of plot doc in the live documentation, just to see that the various plots which are supposed to have aspect ratio 1 really do. Also, did anyone check the examples Dan Drake posted?

I am flying in a few hours and cannot do this while packing. But someone needs to check, if they haven't, that all the major categories of plot at the top of the plot doc still have the correct default aspect ratios and look good. Or verify that this has already been done.

jasongrout commented 12 years ago

comment:25

I agree with kcrisman and jdemeyer; after a night's sleep, I realized that this broke the very first example in plot.py (well, the documentation text wasn't true anymore, which is not something the automated testing could catch).

I posted a patch up at #12222 which corrects the default aspect ratio for objects which make sense to default to aspect ratio 1.0, I've sent a message to sage-devel asking for comment. Let's continue this discussion at #12222, since this ticket is closed now.

sagemath / sage

aspect_ratio and figsize for graphics do not work as expected in sage-4.8.alpha5 #12213