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 )
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)
Change History (24)
Changed 8 years ago by
comment:1 Changed 8 years ago by
comment:2 Changed 8 years ago by
- Status changed from new to needs_info
comment:3 Changed 8 years ago by
- Description modified (diff)
comment:4 Changed 8 years ago by
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
- 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: ↓ 7 Changed 8 years ago by
See also #6249 about the z-order of sums of graphs.
comment:7 in reply to: ↑ 6 Changed 8 years ago by
comment:8 Changed 8 years ago by
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
- Description modified (diff)
comment:10 Changed 8 years ago by
- Priority changed from major to blocker
Considering it breaks a doctest (even if it still "passes")...
comment:11 Changed 8 years ago by
- 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
I'm okay with setting the default for polygon to 'automatic'
comment:13 Changed 8 years ago by
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
Are there any plans to get this fixed soon, considering this has been made a blocker?
comment:15 Changed 8 years ago by
I will work on this at the Sage days starting next Monday. Is that soon enough?
comment:16 Changed 8 years ago by
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
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
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
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
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
comment:21 Changed 8 years ago by
- Description modified (diff)
- Status changed from needs_work to positive_review
comment:22 Changed 8 years ago by
- Merged in set to sage-4.8.rc0
- Resolution set to fixed
- Status changed from positive_review to closed
The list of objects this ticket touches is up for discussion at http://groups.google.com/group/sage-devel/browse_thread/thread/100cbb64787d0ba9