Opened 15 years ago
Closed 11 years ago
#2100 closed enhancement (fixed)
sensible defaults for aspect ratio
Reported by: | Jason Grout | Owned by: | Jason Grout |
---|---|---|---|
Priority: | major | Milestone: | sage-4.7.2 |
Component: | graphics | Keywords: | sd31 |
Cc: | Karl-Dieter Crisman, mhampton, Mitesh Patel | Merged in: | sage-4.7.2.alpha0 |
Authors: | Jason Grout, Karl-Dieter Crisman | Reviewers: | Andrey Novoseltsev, Karl-Dieter Crisman, Ryan Grout |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
We have two concepts of "aspect ratio" that we'd like to expose to the user.
This patch exposes the following behavior:
* Figure aspect ratio -- controls the final image size, including any text labels, etc. This will be set using the following options: o figsize + single number - use this as a base size for the width of the figure canvas + two numbers - an actual figure size in inches + None - use the default size in Matplotlib o fit_tight + if True, clip or extend the resulting figure to the plot objects (so the resulting figure will probably not have figsize dimensions + if False, the resulting figure has exactly figsize dimensions, but items in the plot may be clipped or there may be excessive padding around drawn objects * Pixel aspect ratio o aspect_ratio -- the ratio height/width for a unit square + 1 -- a unit square appears to have equal height and width. This will be the default for Graphics objects (so if a default is not explicitly set, the default is this.) + 'auto' -- plot the given data limits in the given (or computed) figsize, filling the figure (default for plot and list_plot) + number -- ratio of height to width.
To patchbot/reviewers: Apply attachment:trac_2100-aspect-ratio-rebase.patch and attachment:trac_2100-auto-automatic-and-vector.patch
Attachments (6)
Change History (86)
comment:1 Changed 15 years ago by
comment:3 Changed 14 years ago by
Type: | defect → enhancement |
---|
comment:4 Changed 12 years ago by
Report Upstream: | → N/A |
---|
#9813 takes care of two cases of these.
Interestingly, from the above list, it seems like the golden ratio is only used for a minority of functions (i.e., "plot" and "list_plot"). So maybe we ought to change the global default to aspect_ratio=1 and just set plot and list_plot to have default golden ratio aspect ratios.
comment:5 Changed 12 years ago by
Matplotlib aspect ratio setting | Matplotlib adjustable setting | Mathematica AspectRatio setting | Explanation |
'auto' | doesn't seem to matter | Full | Make the current data limits fill the available sized box |
'equal' or 1 | 'box' | Automatic | Make each pixel be square (aspect ratio 1) |
It appears that specifying a number in matplotlib ('box' adjustable) makes the *pixels* have a certain height-to-width ratio (e.g., a circle in the picture will appear to have the given aspect ratio), but in Mathematica, a number specifies the overall ratio of the *image* height-to-width. Thus the numbers are not directly comparable.
Plus, in matplotlib, you can have the 'datalim' be adjustable, which changes the data limits on your plot to make the pixels have a certain aspect ratio.
comment:6 Changed 12 years ago by
Matplotlib's fig_aspect seems to do the sort of thing that mma's aspect ratio number setting does:
http://matplotlib.sourceforge.net/api/figure_api.html#matplotlib.figure.figaspect
comment:7 Changed 12 years ago by
Cc: | Karl-Dieter Crisman added |
---|
comment:8 Changed 12 years ago by
One other note: setting bbox_inches='tight' in the savefig method seems to override the figure size specified.
comment:9 Changed 12 years ago by
We have two concepts of "aspect ratio" that we'd like to expose to the user.
- Figure aspect ratio -- controls the final image size, including any text labels, etc. This will be set using the following options:
- fig_aspect_ratio
- number - make an image with the specified aspect ratio (within reason) using the figaspect matplotlib function (which tries to be marginally smart, which actually may be a dumb thing to do...)
- 'auto' (default) - use bbox_inches='tight' to create a figure that holds the drawn objects
- figsize
- single number - use this as a base size for fig_aspect_ratio calculations (i.e., bigger numbers = bigger figures)
- two numbers - an actual figure size in inches
- fig_aspect_ratio
- Pixel aspect ratio
- aspect_ratio -- the aspect ratio of a pixel in the image (or of a unit square in data coordinates, for example). The default here will change depending on the type of plot
- 'equal' or 1 -- square pixels. This will be the default for most things
- 'auto' -- plot the given data limits in the given (or computed) figsize, filling the figure
- number -- ratio of width to height (or height to width; I can't remember). For plot() and list_plot(), this will default to giving a golden ratio aspect ratio
- aspect_ratio_adjust -- what should we adjust to achieve the desired aspect ratio for the items drawn? Note that by default, the axes limits are enlarged slightly; to eliminate this, set axes_pad=0
- 'box' (default) -- the frame axes
- 'datalim' -- the data limits
- aspect_ratio -- the aspect ratio of a pixel in the image (or of a unit square in data coordinates, for example). The default here will change depending on the type of plot
What do people think?
comment:10 Changed 12 years ago by
Helpful matplotlib mailing list post (read the thread): http://www.mail-archive.com/matplotlib-users@lists.sourceforge.net/msg15623.html
comment:11 Changed 12 years ago by
Description: | modified (diff) |
---|
Moving the proposal up to the description so we can edit it. Ignore the proposal in the comments now.
comment:13 Changed 12 years ago by
comment:14 follow-up: 16 Changed 12 years ago by
Authors: | → Jason Grout |
---|---|
Status: | new → needs_work |
This ticket is still needs_work. Remaining items include:
- Updating docs to reflect changes, including
- Most aspect ratios now default to 1
- new/changed options:
- fig_tight
- the new definition of aspect_ratio (= 1/old definition)
- aspect_ratio_adjustable
- Make GraphicsArray work (likely using the nice features of matplotlib in 1.0 for doing multiple subplots!)
Warning: this patch changes the definition of aspect_ratio. Before, it was width/height, but now it is height/width. The new meaning is consistent with matplotlib and Mathematica.
Since the patch now changes a very fundamental thing about Sage (the meaning of aspect ratio), it probably should be put off until a major release.
comment:15 Changed 12 years ago by
Also todo: set barchart and scatterplot to have default aspect ratios of 'auto'.
comment:16 Changed 12 years ago by
Replying to jason:
Warning: this patch changes the definition of aspect_ratio. Before, it was width/height, but now it is height/width. The new meaning is consistent with matplotlib and Mathematica. Since the patch now changes a very fundamental thing about Sage (the meaning of aspect ratio), it probably should be put off until a major release.
Apparently this statement isn't true. The old aspect_ratio and new aspect_ratio mean the same thing. I was basing the statement on reading the docs. Either the current docs are backwards, or I was misreading them.
So, in short, things are good to go for finishing this patch and getting it into Sage as soon as possible.
Changed 12 years ago by
Attachment: | 2100-aspect_ratio.2.patch added |
---|
apply instead of previous patch
comment:17 Changed 12 years ago by
Status: | needs_work → needs_review |
---|
New patch revamping the aspect ratio calculations in Sage (and defaults!)
comment:18 Changed 12 years ago by
Description: | modified (diff) |
---|
Updating description to reflect changes in the patch.
comment:19 follow-up: 23 Changed 12 years ago by
It seems to me that set_aspect
ratio only accepts "auto" as a string input, while the proposal also lists "equal" as an option.
Would it be possible to extend the functionality so that it is possible to specify either only width or only height of the final figure? I am thinking of using it in conjunction with SageTeX, where it would be natural to ask either for width=\textwidth
or height=0.5\texthight
and have the second dimension determined automatically based on the actual plot and aspect ratio.
comment:20 Changed 12 years ago by
Status: | needs_review → needs_work |
---|---|
Work issues: | → complete documentation |
comment:21 Changed 12 years ago by
Oops, got the answer to the last question already, for some reason they were not crossed in the above comment.
comment:22 Changed 12 years ago by
Cc: | mhampton added |
---|
This might be good bug-day fodder if people are already working on graphics code.
comment:23 Changed 12 years ago by
Reviewers: | → Andrey Novoseltsev |
---|
I hope to take care of novoselt's comments - 'equal' and the documentation - today. This will depend on #7981, #8632, #10244, and #10143.
Would it be possible to extend the functionality so that it is possible to specify either only width or only height of the final figure? I am thinking of using it in conjunction with SageTeX, where it would be natural to ask either for
width=\textwidth
orheight=0.5\texthight
and have the second dimension determined automatically based on the actual plot and aspect ratio.
I think this is a good idea, but I don't think the people who might work on this will have time for it before bitrot sets in even further. I have opened ticket #10633 for this.
comment:24 Changed 12 years ago by
Rather impressively, only one of seven hunks failed in contour_plot.py, and only 5 of 34 hunks failed on plot.py. So rebasing this to the other recent graphics patches should be no problem, as well as the other documentation comments.
comment:25 Changed 12 years ago by
Let's just take the 'equal' option out of the proposal. I think it's there just because that's what matplotlib does.
comment:26 follow-up: 28 Changed 12 years ago by
I had to change a few things based on the new save() but otherwise nearly all is the same. It also turned out that fig_tight was never used in .matplotlib(), and in fact only can be passed to matplotlib's savefig(), so I treated it basically the same as transparent in .save().
I can also remove 'equal' if you want, that's fine.
Before I go ahead and update this, though, I have a question. The two functions not documented (pad_for_tick_labels
and adjust_figure_to_contain_bbox
) only are used in a commented-out line
# this messes up the aspect ratio! #figure.canvas.mpl_connect('draw_event', pad_for_tick_labels)
So... should they even be included at this time?
comment:27 Changed 12 years ago by
Description: | modified (diff) |
---|
comment:28 follow-up: 29 Changed 12 years ago by
Replying to kcrisman:
I had to change a few things based on the new save() but otherwise nearly all is the same. It also turned out that fig_tight was never used in .matplotlib(), and in fact only can be passed to matplotlib's savefig(), so I treated it basically the same as transparent in .save().
I can also remove 'equal' if you want, that's fine.
Done as well.
Before I go ahead and update this, though, I have a question. The two functions not documented (
pad_for_tick_labels
andadjust_figure_to_contain_bbox
) only are used in a commented-out line# this messes up the aspect ratio! #figure.canvas.mpl_connect('draw_event', pad_for_tick_labels)So... should they even be included at this time?
Quote from Jason: "Feel free to move those functions to another ticket if they are really not called from anywhere. I think they are leftovers from experimentation, so probably not strictly needed (especially if they really aren't called or used from anywhere)."
New patch coming up relatively soon.
comment:29 Changed 12 years ago by
So... should they even be included at this time?
Quote from Jason: "Feel free to move those functions to another ticket if they are really not called from anywhere. I think they are leftovers from experimentation, so probably not strictly needed (especially if they really aren't called or used from anywhere)."
Since I didn't really know where these should be used, or what the context was, I decided to just leave them in but fully commented out, with an admonition to add documentation if they ever get used again. (This would also make it easier for a future developer to use them, inter alia.)
Getting some doctest failures, though, related to this:
sage -t devel/sage/sage/plot/plot3d/plot_field3d.py # 6 doctests failed
I doubt this will be hard to resolve. Anyway, just status updates...
Changed 12 years ago by
Attachment: | trac_2100-aspect-ratio-rebase.patch added |
---|
Rebase to 4.6.1/4.6.2.alpha0
comment:30 follow-up: 32 Changed 12 years ago by
Authors: | Jason Grout → Jason Grout, Karl-Dieter Crisman |
---|---|
Reviewers: | Andrey Novoseltsev → Andrey Novoseltsev, Karl-Dieter Crisman |
Status: | needs_work → needs_review |
The problem with the plot_field3d
turns out to be the following very bad behavior of plotting vectors after this patch
sage: v = vector([1.,2.,3.]) sage: v.plot() # all is well sage: plot(v) # passes in aspect_ratio to a 3d graphics, which already has such a thing, and get nasty traceback
The culprit was telling plot_vector_field3d
to do plot(v)
for each vector instead of v.plot()
as it should have. However, this exposes something else really dumb - namely, that plot.py
turns vectors into tuples before it plots them
from sage.structure.element import is_Vector if kwds.get('parametric',False) and is_Vector(funcs): funcs = tuple(funcs) if hasattr(funcs, 'plot'): G = funcs.plot(*args, **original_opts) # if we are using the generic plotting method else:
this presumably avoids the fact that
sage: w = vector([x^2,3,x^3]) sage: plot(w,(x,0,1)) --------------------------------------------------------------------------- NotImplementedError
but still this is a problem. So after fixing this issue, I'm opening a ticket for making vector plotting more sensible.
I think this change might need review (?) so I'm setting to 'needs review'.
To buildbot and reviewer: apply trac_2100-aspect-ratio-rebase.patch and trac_2100-3d-vector.patch. Depends on #7981, #8632, #10244, and #10143.
comment:31 Changed 12 years ago by
Work issues: | complete documentation |
---|
comment:32 Changed 12 years ago by
I think this change might need review (?) so I'm setting to 'needs review'.
Oh yeah, and I never actually looked at very many of the plots. Definitely still needs review ;) of the pictures and the (small) reviewer patch.
comment:34 follow-up: 35 Changed 12 years ago by
Status: | needs_review → positive_review |
---|
I'm setting this to positive review - it is a great improvement and it is a shame that it took us so long ;-)
I am fine with the reviewer patch and I did take a look at a few (although definitely not all) plots in the documentation. Circles are finally circles!!!
I have also discovered the following behaviour which may be a bug:
sage: x,y = var('x,y') sage: print x, y sage: f(x,y) = x^2 + y^2 - 2 sage: implicit_plot(f, (-3, 3), (-3, 3),fill=True).show(aspect_ratio=1) sage: implicit_plot(f, (-3, 3), (-3, 3),fill=False)
The first line (with fill=True
) completely fills the plot, not just the disk.
comment:35 Changed 12 years ago by
Thanks!
I have also discovered the following behaviour which may be a bug:
sage: x,y = var('x,y') sage: print x, y sage: f(x,y) = x^2 + y^2 - 2 sage: implicit_plot(f, (-3, 3), (-3, 3),fill=True).show(aspect_ratio=1) sage: implicit_plot(f, (-3, 3), (-3, 3),fill=False)The first line (with
fill=True
) completely fills the plot, not just the disk.
This also happens with e.g. 4.6.1.alpha3, so this is definitely not related. It sounds a LOT like #9744. It'd be great if you could confirm that. I thought we'd already taken care of this... unfortunately not, it seems.
comment:36 follow-up: 38 Changed 12 years ago by
Status: | positive_review → needs_work |
---|---|
Work issues: | → Plotting of vectors examples |
Actually, I was too hasty:
sage -t -long "devel/sage-main/sage/modules/free_module_element.pyx" ********************************************************************** File "/mnt/usb1/scratch/novoselt/sage/devel/sage-main/sage/modules/free_module_element.pyx", line 1390: sage: plot(v) # defaults to an arrow plot Exception raised: Traceback (most recent call last): File "/mnt/usb1/scratch/novoselt/sage/local/bin/ncadoctest.py", line 1231, in run_one_test self.run_one_example(test, example, filename, compileflags) File "/mnt/usb1/scratch/novoselt/sage/local/bin/sagedoctest.py", line 38, in run_one_example OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags) File "/mnt/usb1/scratch/novoselt/sage/local/bin/ncadoctest.py", line 1172, in run_one_example compileflags, 1) in test.globs File "<doctest __main__.example_45[9]>", line 1, in <module> plot(v) # defaults to an arrow plot###line 1390: sage: plot(v) # defaults to an arrow plot File "/mnt/usb1/scratch/novoselt/sage/local/lib/python/site-packages/sage/misc/displayhook.py", line 174, in displayhook print_obj(sys.stdout, obj) File "/mnt/usb1/scratch/novoselt/sage/local/lib/python/site-packages/sage/misc/displayhook.py", line 142, in print_obj print >>out_stream, `obj` File "base.pyx", line 80, in sage.plot.plot3d.base.Graphics3d.__repr__ (sage/plot/plot3d/base.c:2453) self.show() File "base.pyx", line 1048, in sage.plot.plot3d.base.Graphics3d.show (sage/plot/plot3d/base.c:9375) opts = self._process_viewing_options(kwds) File "base.pyx", line 953, in sage.plot.plot3d.base.Graphics3d._process_viewing_options (sage/plot/plot3d/base.c:9174) self._determine_frame_aspect_ratio(opts['aspect_ratio']) File "base.pyx", line 199, in sage.plot.plot3d.base.Graphics3d._determine_frame_aspect_ratio (sage/plot/plot3d/base.c:3397) return [(a_max[i] - a_min[i])*aspect_ratio[i] for i in range(3)] TypeError: can't multiply sequence by non-int of type 'float' ********************************************************************** File "/mnt/usb1/scratch/novoselt/sage/devel/sage-main/sage/modules/free_module_element.pyx", line 1391: sage: plot(v, plot_type='arrow') Exception raised: Traceback (most recent call last): File "/mnt/usb1/scratch/novoselt/sage/local/bin/ncadoctest.py", line 1231, in run_one_test self.run_one_example(test, example, filename, compileflags) File "/mnt/usb1/scratch/novoselt/sage/local/bin/sagedoctest.py", line 38, in run_one_example OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags) File "/mnt/usb1/scratch/novoselt/sage/local/bin/ncadoctest.py", line 1172, in run_one_example compileflags, 1) in test.globs File "<doctest __main__.example_45[10]>", line 1, in <module> plot(v, plot_type='arrow')###line 1391: sage: plot(v, plot_type='arrow') File "/mnt/usb1/scratch/novoselt/sage/local/lib/python/site-packages/sage/misc/displayhook.py", line 174, in displayhook print_obj(sys.stdout, obj) File "/mnt/usb1/scratch/novoselt/sage/local/lib/python/site-packages/sage/misc/displayhook.py", line 142, in print_obj print >>out_stream, `obj` File "base.pyx", line 80, in sage.plot.plot3d.base.Graphics3d.__repr__ (sage/plot/plot3d/base.c:2453) self.show() File "base.pyx", line 1048, in sage.plot.plot3d.base.Graphics3d.show (sage/plot/plot3d/base.c:9375) opts = self._process_viewing_options(kwds) File "base.pyx", line 953, in sage.plot.plot3d.base.Graphics3d._process_viewing_options (sage/plot/plot3d/base.c:9174) self._determine_frame_aspect_ratio(opts['aspect_ratio']) File "base.pyx", line 199, in sage.plot.plot3d.base.Graphics3d._determine_frame_aspect_ratio (sage/plot/plot3d/base.c:3397) return [(a_max[i] - a_min[i])*aspect_ratio[i] for i in range(3)] TypeError: can't multiply sequence by non-int of type 'float' ********************************************************************** File "/mnt/usb1/scratch/novoselt/sage/devel/sage-main/sage/modules/free_module_element.pyx", line 1393: sage: plot(v, plot_type='point')+frame3d((0,0,0), v.list()) Exception raised: Traceback (most recent call last): File "/mnt/usb1/scratch/novoselt/sage/local/bin/ncadoctest.py", line 1231, in run_one_test self.run_one_example(test, example, filename, compileflags) File "/mnt/usb1/scratch/novoselt/sage/local/bin/sagedoctest.py", line 38, in run_one_example OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags) File "/mnt/usb1/scratch/novoselt/sage/local/bin/ncadoctest.py", line 1172, in run_one_example compileflags, 1) in test.globs File "<doctest __main__.example_45[12]>", line 1, in <module> plot(v, plot_type='point')+frame3d((Integer(0),Integer(0),Integer(0)), v.list())###line 1393: sage: plot(v, plot_type='point')+frame3d((0,0,0), v.list()) File "/mnt/usb1/scratch/novoselt/sage/local/lib/python/site-packages/sage/misc/displayhook.py", line 174, in displayhook print_obj(sys.stdout, obj) File "/mnt/usb1/scratch/novoselt/sage/local/lib/python/site-packages/sage/misc/displayhook.py", line 142, in print_obj print >>out_stream, `obj` File "base.pyx", line 80, in sage.plot.plot3d.base.Graphics3d.__repr__ (sage/plot/plot3d/base.c:2453) self.show() File "base.pyx", line 1048, in sage.plot.plot3d.base.Graphics3d.show (sage/plot/plot3d/base.c:9375) opts = self._process_viewing_options(kwds) File "base.pyx", line 953, in sage.plot.plot3d.base.Graphics3d._process_viewing_options (sage/plot/plot3d/base.c:9174) self._determine_frame_aspect_ratio(opts['aspect_ratio']) File "base.pyx", line 199, in sage.plot.plot3d.base.Graphics3d._determine_frame_aspect_ratio (sage/plot/plot3d/base.c:3397) return [(a_max[i] - a_min[i])*aspect_ratio[i] for i in range(3)] TypeError: can't multiply sequence by non-int of type 'float' ********************************************************************** 1 items had failures: 3 of 17 in __main__.example_45 ***Test Failed*** 3 failures.
These are the only errors on the whole library. I guess they should be fixed in the same way as in the reviewer patch. Or maybe there should be a better fix?..
comment:37 Changed 12 years ago by
Yes, the problem I hit is exactly #9744 (which actually mentions the same example from the documentation), thanks for pointing it out.
comment:38 follow-up: 39 Changed 12 years ago by
Replying to novoselt:
Actually, I was too hasty: sage -t -long "devel/sage-main/sage/modules/free_module_element.pyx"
Aargh! I'll try to fix this...
These are the only errors on the whole library. I guess they should be fixed in the same way as in the reviewer patch. Or maybe there should be a better fix?..
No, that is the correct fix for now. An overarching ticket is #10638, which I forgot to mention before.
comment:39 follow-up: 40 Changed 12 years ago by
Replying to kcrisman:
Replying to novoselt:
Actually, I was too hasty: sage -t -long "devel/sage-main/sage/modules/free_module_element.pyx"
Aargh! I'll try to fix this...
These are the only errors on the whole library. I guess they should be fixed in the same way as in the reviewer patch. Or maybe there should be a better fix?..
No, that is the correct fix for now. An overarching ticket is #10638, which I forgot to mention before.
The correct fix is to remove that kwd from the plot when
if plot_type == 'arrow' or plot_type == 'point': dimension = len(coords) if dimension == 3: from sage.plot.plot3d.shapes2 import line3d, point3d if plot_type == 'arrow': return line3d([(0,0,0), coords], arrow_head=True, **kwds) else: return point3d(coords, **kwds)
that should do it. I will try this later.
comment:40 Changed 12 years ago by
These are the only errors on the whole library. I guess they should be fixed in the same way as in the reviewer patch. Or maybe there should be a better fix?..
No, that is the correct fix for now. An overarching ticket is #10638, which I forgot to mention before.
Okay, now I am not rushing out the door and can think straight. The problem is that this was never doctesting the thing in question in the first place because of the issues raised in #10638.
I think the issue is that _extract_kwds_for_show
keeps the @options
value of aspect_ratio='auto') in play; when this gets passed to the
show() method of
Line (which is what
line3d creates in this case), it refers to the one in
base.PrimitiveObject?, which unfortunately then puts this value of
aspect_ratio` in:
opts = {} opts.update(SHOW_DEFAULTS) if self._extra_kwds is not None: opts.update(self._extra_kwds) opts.update(kwds)
and then doesn't realize it's a problem in
if opts['frame_aspect_ratio'] == 'automatic': if opts['aspect_ratio'] != 'automatic': # Set the aspect_ratio of the frame to be the same as that of # the object we are rendering given the aspect_ratio we'll use # for it. opts['frame_aspect_ratio'] = \ self._determine_frame_aspect_ratio(opts['aspect_ratio'])
Which is where the problem happens.
Although fixing this in the vector plotting didn't seem to work immediately.
Unless we wanted to change all the 'auto' in this patch to 'automatic'. I don't know if that is desirable either, though. What do others think?
comment:41 follow-up: 42 Changed 12 years ago by
I like auto
more than automatic
, so I don't want to change it.
Can we maybe change it the other way? Was automatic
documented/used somewhere before?
comment:42 Changed 12 years ago by
I like
auto
more thanautomatic
, so I don't want to change it.
Agreed, but...
Can we maybe change it the other way? Was
automatic
documented/used somewhere before?
Yes, this is the standard 3D option. See this reference manual page for plot/plot3d/base.pyx.
Of course, it would be possible to change the plot3d case to also accept 'auto' easily enough - in fact, that's essentially what I've done in my latest patch - and one could change the documentation accordingly. But we couldn't remove it without a deprecation period - and in my mind it's pointless to go to the trouble, given that it's mostly a default so we could change things without even needing to deprecate that option.
comment:43 follow-up: 44 Changed 12 years ago by
I guess then changing to automatic
is OK.
For consistency, it would be nice if the same variant was used throughout Sage, but then again the point is that this value should not be specified explicitly by the user.
comment:44 follow-up: 45 Changed 12 years ago by
Replying to novoselt:
I guess then changing to
automatic
is OK.
I'd want to run that by Jason (the original author) first, though - after all, the point below would suggest it doesn't matter (and it is a long process for me to rebase, sadly).
For consistency, it would be nice if the same variant was used throughout Sage, but then again the point is that this value should not be specified explicitly by the user.
comment:45 Changed 12 years ago by
I guess then changing to
automatic
is OK.I'd want to run that by Jason (the original author) first, though - after all, the point below would suggest it doesn't matter (and it is a long process for me to rebase, sadly).
One reason being that matplotlib uses 'auto', and consistency with that is nice as well.
For consistency, it would be nice if the same variant was used throughout Sage, but then again the point is that this value should not be specified explicitly by the user.
comment:46 Changed 12 years ago by
Status: | needs_work → needs_review |
---|
Okay, this latest reviewer patch should fix the things we've talked about in a minimalist way. It doesn't resolve the 'auto' versus 'automatic' issue. No new instructions for applying patches. Documentation also looks good with this change.
comment:47 Changed 12 years ago by
Work issues: | Plotting of vectors examples |
---|
comment:48 follow-up: 49 Changed 12 years ago by
Wow, great job tracking down this subtle problem. I personally slightly prefer 'auto'
over 'automatic'
because it's consistent with matplotlib and it's shorter. However, I agree that users will probably not specify it too frequently, so 'automatic'
would probably be fine as well. If we do make the default 'automatic'
, then I think we'll have to convert it to 'auto'
before passing it to matplotlib.
Another data point: mma uses Automatic (not Auto): http://reference.wolfram.com/mathematica/ref/Automatic.html
comment:49 Changed 12 years ago by
Replying to jason:
Wow, great job tracking down this subtle problem. I personally slightly prefer
'auto'
over'automatic'
because it's consistent with matplotlib and it's shorter. However, I agree that users will probably not specify it too frequently, so'automatic'
would probably be fine as well.
Thanks. I don't have any more time today to work on this, but I think then I vote for 'automatic'
for consistency, and having 'auto'
as one that secretly also works :)
If we do make the default
'automatic'
, then I think we'll have to convert it to'auto'
before passing it to matplotlib.
Yes, I realize that. I don't think that will be very difficult, since there seems to be only one place this is passed.
comment:50 Changed 12 years ago by
Status: | needs_review → needs_work |
---|---|
Work issues: | → unify option names |
Okay, putting this to 'needs work' in order to fix this. Hopefully today or this weekend; it won't be hard, just have to do it.
comment:51 Changed 12 years ago by
Oops... hmm, I gave myself one long weekend. I'll try to see if this still applies to 4.6.2.alpha4 soon.
comment:52 Changed 12 years ago by
All still applies to 4.6.2.alpha4. I hope to conclude this adventure shortly, by replacing the reviewer patch with one that deals with the automatic/auto issue uniformly.
comment:53 follow-up: 54 Changed 12 years ago by
Description: | modified (diff) |
---|---|
Status: | needs_work → needs_review |
This should all work fine now. I do want to point out that the standard plots seem to have increased somewhat in size. The new reviewer patch is now different enough that it needs review. Please do so now to avoid bitrot!
comment:54 Changed 12 years ago by
Replying to kcrisman:
This should all work fine now. I do want to point out that the standard plots seem to have increased somewhat in size.
Yes, I reverted things to the matplotlib defaults. Before, we made the ratio of height/width the golden ratio, but now it is the standard matplotlib default (which I believe is 4:3, which matches computer screens better anyway).
comment:55 Changed 12 years ago by
Owner: | changed from William Stein to Jason Grout |
---|
Can you add instructions for reviewers/patchbot about which patches should be applied in what order?
comment:57 Changed 12 years ago by
Description: | modified (diff) |
---|
See the description. I guess I didn't make it obvious enough :)
Apply [trac_2100-aspect-ratio-rebase.patch] and [trac_2100-auto-automatic-and-vector.patch] for this.
comment:58 follow-up: 60 Changed 12 years ago by
Description: | modified (diff) |
---|
Apply [trac_2100-aspect-ratio-rebase.patch trac_2100-aspect-ratio-rebase.patch] and [trac_2100-auto-automatic-and-vector.patch trac_2100-auto-automatic-and-vector.patch] for this.
comment:60 Changed 12 years ago by
Description: | modified (diff) |
---|
To patchbot/reviewers: Apply attachment:trac_2100-aspect-ratio-rebase.patch and attachment:trac_2100-auto-automatic-and-vector.patch
comment:61 Changed 12 years ago by
Here is some code that looks less good with this patch than before.
def my_eulers_method_plot(a_function,x0,y0,h,x1): n=int((1.0)*(x1-x0)/h) x00=x0; y00=y0 x01=x0; y01=y0 P=point((x00,y00),rgbcolor=hue(1)) # red Q=Graphics() # default is blue f(x,y)=a_function(x,y) # Note that a_function should be a callable function of x, then y for i in range(n+1): y01 = y00+h*f(x00,y00) x01 = x00+h P=P+point((x01,y01),rgbcolor=hue(1)) Q=Q+line([(x00,y00),(x01,y01)]) x00=x01 y00=y01 return(P+Q) var('x,y') def euler_logistic_plot(parameter,y_start,end=15,step=1): function(x,y)=parameter*y*(1-y) my_eulers_method_plot(function,0,y_start,step,end).show(ymin=0) euler_logistic_plot(2,.7,97,.8)
I'll try to attach before and after.
comment:62 Changed 12 years ago by
Status: | needs_review → needs_info |
---|
The issue is of course that I never used plot()
to do this, but rather only line()
and point()
and Graphics()
. The default aspect ratio seems to have changed a lot, and so not only are the labels on the left totally squished, but it just looks bad. Maybe mpl defaults aren't that great? Or is this a pretty unusual scenario?
Anyway, putting as needs info until I know what is happening here from someone who knows more about mpl (e.g., Jason). So frustrating, too, because we should have this in!
comment:63 Changed 12 years ago by
Yep, I think the problem here is that line and point now default to have aspect_ratio=1, so the y and x axis are drawn at equal scales. Try setting the aspect_ratio to 'auto' (or whatever your patch does now).
plot() defaults to 'auto' aspect ratio (e.g., fill the figure). However, geometric figures like line and point default to aspect_ratio=1.
comment:64 Changed 12 years ago by
I think perhaps we should make line (and point?) default to 'auto'? Because one is quite likely to have a line do a function, as opposed to other geometric figures. And points are often used to shadow functions.
Any other thoughts on this? It would be really nice to finally get this in 4.7 - maybe you can take a quick look over spring break?
comment:65 Changed 12 years ago by
Status: | needs_info → needs_review |
---|---|
Work issues: | unify option names |
Putting needs review, though may need a minor change because of the thing in the last two comments.
comment:66 Changed 11 years ago by
Keywords: | sd31 added |
---|
Okay, I'm going to refresh so that line and point still have 'auto'. Needs review!
comment:67 Changed 11 years ago by
Reviewers: | Andrey Novoseltsev, Karl-Dieter Crisman → Andrey Novoseltsev, Karl-Dieter Crisman, Ryan Grout |
---|
FYI, I am currently reviewing this patch
comment:68 follow-up: 69 Changed 11 years ago by
Status: | needs_review → positive_review |
---|
Patch looks good. Confirmed that kcrisman's code looks 'good' after the patch too :) Positive Review. Great job guys.
Note: I do get doctest failures on plot_field.py, but the failures are not caused by the patches to this ticket.
comment:69 Changed 11 years ago by
Replying to ryan:
Note: I do get doctest failures on plot_field.py, but the failures are not caused by the patches to this ticket.
What exactly do you mean by this? That a clean public release of Sage has doctest failures in plot_field.py
?
comment:70 Changed 11 years ago by
Correct - or to be more precise, warnings that do not cause the doctest to fail. They are known, and not from Sage.
comment:71 Changed 11 years ago by
Milestone: | sage-4.7.1 → sage-4.7.2 |
---|
comment:72 follow-up: 73 Changed 11 years ago by
Status: | positive_review → needs_work |
---|---|
Work issues: | → rebase |
Please rebase this patch to be applied on top of #11491.
comment:73 follow-up: 75 Changed 11 years ago by
Replying to jdemeyer:
Please rebase this patch to be applied on top of #11491.
Thanks - another rebase where a major improvement takes second rank to a trivial adjustment. Especially since Ryan and I were involved on both tickets, it would have been nice to ask first. Rebasing is not so trivial for the setups some of us have.
But no point crying over spilt milk. Sigh... patches coming up.
Changed 11 years ago by
Attachment: | trac_2100-auto-automatic-and-vector.patch added |
---|
Apply after rebase patch
comment:74 Changed 11 years ago by
Status: | needs_work → positive_review |
---|
Okay, now we should hopefully be at positive review!
comment:75 Changed 11 years ago by
Replying to kcrisman:
Especially since Ryan and I were involved on both tickets, it would have been nice to ask first.
I have done that in the past and people complained: http://groups.google.com/group/sage-devel/browse_frm/thread/abd5fb944769e052/8e4057172b97f2e5
comment:76 follow-ups: 77 78 Changed 11 years ago by
Right, and my point is that this sort of feels the same way. I am not suggesting unmerging something positively reviewed - definitely not the direction I'm going.
What I'm suggesting (and only suggesting, because I can only imagine how much work release management is) is that perhaps sending a ping via Trac on two closely related tickets which have some overlap would be good before deciding which one to merge first. Especially since in this case #11491 is so clearly much more trivial than this one, and also much older. I'm sure Ryan and I would have agreed that this one was higher priority.
Anyway, it turned out I had time for fixing it - it was a very small overlap, luckily. Thanks for all the hard work; the final releases really are noticeably more polished nowadays, and it's a great thing.
comment:77 follow-up: 79 Changed 11 years ago by
Work issues: | rebase |
---|
Replying to kcrisman:
Right, and my point is that this sort of feels the same way. I am not suggesting unmerging something positively reviewed - definitely not the direction I'm going.
What I'm suggesting (and only suggesting, because I can only imagine how much work release management is) is that perhaps sending a ping via Trac on two closely related tickets which have some overlap would be good before deciding which one to merge first. Especially since in this case #11491 is so clearly much more trivial than this one, and also much older. I'm sure Ryan and I would have agreed that this one was higher priority.
It is not easy for me to determine a priori which patches conflict with eachother, so what you propose isn't really possible. It's not that I looked at both patches and decided which one to merge. What happened is that I already decided to merge #11491 before I even considered the patch here.
Since the sage-devel discussion I mentioned, there is an agreement that once a patch is merged (even in a future alpha version), it should stay merged if possible.
comment:78 Changed 11 years ago by
Cc: | Mitesh Patel added |
---|
comment:79 Changed 11 years ago by
It is not easy for me to determine a priori which patches conflict with eachother, so what you propose isn't really possible. It's not that I looked at both patches and decided which one to merge. What happened is that I already decided to merge #11491 before I even considered the patch here.
Understood.
Since the sage-devel discussion I mentioned, there is an agreement that once a patch is merged (even in a future alpha version), it should stay merged if possible.
Yes, that definitely makes sense, as I hope I make clear above. :)
comment:80 Changed 11 years ago by
Merged in: | → sage-4.7.2.alpha0 |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
For reference, in Mathematica (see http://reference.wolfram.com/mathematica/ref/AspectRatio.html?q=AspectRatio&lang=en )
Automatic is an aspect ratio determined by the pixels in the plot. It does not distort the plot (i.e., circles are still circles).