Opened 8 years ago

Closed 8 years ago

#12222 closed defect (fixed)

default aspect ratio for geometric objects

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

Description (last modified by kcrisman)

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 8 years ago.
trac_12222-reviewer.patch (914 bytes) - added by kcrisman 8 years ago.

Download all attachments as: .zip

Change History (24)

Changed 8 years ago by jason

comment:1 Changed 8 years ago by jason

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 8 years ago by jason

  • Status changed from new to needs_info

comment:3 Changed 8 years ago by jason

  • Description modified (diff)

comment:4 Changed 8 years ago by novoselt

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 8 years ago by jason

  • Status changed from needs_info to needs_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 follow-up: Changed 8 years ago by vbraun

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

comment:7 in reply to: ↑ 6 Changed 8 years ago by kcrisman

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

And, as I point out there, #3251.

comment:8 Changed 8 years ago by jason

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

comment:9 Changed 8 years ago by jason

  • Description modified (diff)

comment:10 Changed 8 years ago by kcrisman

  • Priority changed from major to blocker

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

comment:11 Changed 8 years ago by kcrisman

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

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

comment:13 Changed 8 years ago by kcrisman

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

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

comment:15 Changed 8 years ago by jason

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

comment:16 Changed 8 years ago by kcrisman

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

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

comment:18 Changed 8 years ago by kcrisman

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

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

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

comment:21 Changed 8 years ago by kcrisman

  • Description modified (diff)
  • Status changed from needs_work to positive_review

comment:22 Changed 8 years ago by jdemeyer

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