Opened 11 years ago

Closed 11 years ago

#12222 closed defect (fixed)

default aspect ratio for geometric objects

Reported by: Jason Grout Owned by: jason, was
Priority: blocker Milestone: sage-4.8
Component: graphics Keywords:
Cc: William Stein, Karl-Dieter Crisman Merged in: sage-4.8.rc0
Authors: Jason Grout Reviewers: Karl-Dieter Crisman
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by Karl-Dieter Crisman)

This ticket is about changing the default aspect ratio for these objects to 1.0:

  • circle
  • arc
  • disk
  • ellipse
  • polygon

The idea is that when someone draws these, they want angles and circles to look correct in the plot. Changing the default aspect ratio for a circle also corrects the very first example in plot.py (which was broken by #12213)

Apply trac-12222-default-aspect-ratio.patch and trac_12222-reviewer.patch.

Attachments (2)

trac-12222-default-aspect-ratio.patch (5.6 KB) - added by Jason Grout 11 years ago.
trac_12222-reviewer.patch (914 bytes) - added by Karl-Dieter Crisman 11 years ago.

Download all attachments as: .zip

Change History (24)

Changed 11 years ago by Jason Grout

comment:1 Changed 11 years ago by Jason Grout

The list of objects this ticket touches is up for discussion at http://groups.google.com/group/sage-devel/browse_thread/thread/100cbb64787d0ba9

comment:2 Changed 11 years ago by Jason Grout

Status: newneeds_info

comment:3 Changed 11 years ago by Jason Grout

Description: modified (diff)

comment:4 Changed 11 years ago by Andrey Novoseltsev

Let me repeat a suggestion from #12213: have a distinction between "user set aspect ratio 1" and "automatically set aspect ratio 1". In the first case we definitely must do what the user wants. In the second case it is reasonable to skew circles, if otherwise the plot will be totally horrible: something like having one dimension 5 times bigger than another.

If this is implemented, I would argue that the default always should be 1, except for cases when there is a good reason otherwise.

We think that if someone draws a circle, it is expected to be round, but for lines nobody cares.

However, if I draw two lines with slopes 2 and -1/2, I expect to see that they are perpendicular, and if they are not - it is the same as a skew circle.

It also seems to me that plot addition does not have to be associative, especially for 2D plots: to me left + right means "draw right on top of left" and it seemed to me that's what it does.

With this convention, it actually may make sense to inherit the aspect ratio from the left (which is likely to be more complicated, especially in "+=" operations). I unintentionally expected it when I had a complicated plot and wanted to add a couple lines to it. When the first plot is obtained from some function and it also has say, label options adjusted in a certain way, it gets especially annoying that adding a single line or a point discards all the customized look, unless I repeat all adjustments (which were hidden in the function).

comment:5 Changed 11 years ago by Jason Grout

Status: needs_infoneeds_review

It sounds like a good idea to have better heuristics. I guess the tricky part is being smart enough to be nice, but not too smart that it is a guess what is going to happen. Can we open a ticket and discussion about this suggestion, since it is going far beyond the rather limited purpose of this ticket, which is to repair some common use case problems that #12213 breaks?

comment:6 Changed 11 years ago by Volker Braun

See also #6249 about the z-order of sums of graphs.

comment:7 in reply to:  6 Changed 11 years ago by Karl-Dieter Crisman

See also #6249 about the z-order of sums of graphs.

And, as I point out there, #3251.

comment:8 Changed 11 years ago by Jason Grout

Can I ask again for a review of this conservative, but important, fix for some unintended consequences of #12213?

comment:9 Changed 11 years ago by Jason Grout

Description: modified (diff)

comment:10 Changed 11 years ago by Karl-Dieter Crisman

Priority: majorblocker

Considering it breaks a doctest (even if it still "passes")...

comment:11 Changed 11 years ago by Karl-Dieter Crisman

Reviewers: Karl-Dieter Crisman
Status: needs_reviewneeds_work

In general, this is great. So almost ready for prime time.

Okay, here's something that still doesn't look good and is in the documentation already.

In plot.py:

f(x) = (x-3)*(x-5)*(x-7)+40
P = line([(2,0),(2,f(2))], color='black')
P += line([(8,0),(8,f(8))], color='black')
P += polygon([(2,0),(2,f(2))] + [(x, f(x)) for x in [2,2.1,..,8]] + [(8,0),(2,0)],  rgbcolor=(0.8,0.8,0.8))
P += text("$\\int_{a}^b f(x) dx$", (5, 20), fontsize=16, color='black')
P += plot(f, (1, 8.5), thickness=3)
P    # show the result

The polygon one needs to have something done with it, because it's overriding the 'automatic' and looks quite bad.

Hopefully this is the only one. Putting to 'needs work', but don't whip up a patch immediately until I've checked the rest.

I have to say, reading through the plot doc page with pictures is quite pleasant visually.

comment:12 Changed 11 years ago by Jason Grout

I'm okay with setting the default for polygon to 'automatic'

comment:13 Changed 11 years ago by Karl-Dieter Crisman

I wasn't suggesting setting default for polygon to 'automatic', but to use it in this particular example. I think it's a little unusual - usually one would use the fill keyword, not the polygon, for this case!

A few more things:

  • I don't know why, but I'm still getting aspect ratio automatic pictures in arcs even though the aspect ratio test in the documentation is passing.
  • I have a feeling that complex plots should have 'automatic' as well, now, if that's possible to do easily (it may not be, and if it's not new then forget about it). Try, for instance, complex_plot(sqrt(x), (-5, 5), (-.1, .1)).
  • Contour plots and vector fields seem to have 'automatic' now (try contour_plot(f, (-1, 1), (-200, 200), contours=(0.1, 1.0, 1.2, 1.4), cmap='hsv')) but density plots clearly are 1.0. I'm not sure which is better, but they should be the same, I think. I seem to recall them being 1.0 before, but of course all the examples are ones where 1.0 and automatic will look the same.

comment:14 Changed 11 years ago by Jeroen Demeyer

Are there any plans to get this fixed soon, considering this has been made a blocker?

comment:15 Changed 11 years ago by Jason Grout

I will work on this at the Sage days starting next Monday. Is that soon enough?

comment:16 Changed 11 years ago by Karl-Dieter Crisman

I would be okay with fixing the doctest in comment:11 by adding the 'automatic' in that example for the polygon (assuming we have another ticket to fix the underlying problem) and getting aspect ratio 1.0 pictures in arcs. The first is trivial, and hopefully the second wouldn't be too hard - must be a missing option decorator somewhere - maybe (?) it would be a good thing to show people at the Sage booth how we fix Sage... ?

The other things are something we'd want to think about on another ticket, as no current doctests break.

comment:17 Changed 11 years ago by Jeroen Demeyer

Considering this is a blocker ticket, you should really aim for a minimal fix: simply fix the blocker issue, nothing more.

comment:18 Changed 11 years ago by Karl-Dieter Crisman

Okay, but then we still have changed a doctest (to be wrong) and comment:11 needs to be fixed. I would argue that William's ticket breaking the arcs is bad too, but I suppose I could live with this just so 4.8 can be out.

If I have enough internet today, I may fix this.

comment:19 Changed 11 years ago by Karl-Dieter Crisman

Okay, fixing comment:11 is easy. I cannot figure out why arcs don't work but ellipses don't; the code seems identical. I'll ask Jason later this morning.

comment:20 Changed 11 years ago by Karl-Dieter Crisman

Okay, Jason and I figured out that the ellipse thing is not a bug, it's me being dense. (The axes don't intersect!)

So upcoming reviewer patch gives positive review - Jason gives verbal consent after seeing the fix :)

Changed 11 years ago by Karl-Dieter Crisman

Attachment: trac_12222-reviewer.patch added

comment:21 Changed 11 years ago by Karl-Dieter Crisman

Description: modified (diff)
Status: needs_workpositive_review

comment:22 Changed 11 years ago by Jeroen Demeyer

Merged in: sage-4.8.rc0
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.