-
-
Notifications
You must be signed in to change notification settings - Fork 449
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
sensible defaults for aspect ratio #2100
Comments
comment:1
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). |
comment:2
Jason, didn't we fix this recently? Cheers, Michael |
comment:4
#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
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
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:8
One other note: setting bbox_inches='tight' in the savefig method seems to override the figure size specified. |
comment:9
We have two concepts of "aspect ratio" that we'd like to expose to the user.
What do people think? |
comment:10
Helpful matplotlib mailing list post (read the thread): !http://www.mail-archive.com/matplotlib-users@lists.sourceforge.net/msg15623.html |
This comment has been minimized.
This comment has been minimized.
comment:11
Moving the proposal up to the description so we can edit it. Ignore the proposal in the comments now. |
This comment has been minimized.
This comment has been minimized.
comment:12
Update the proposal. |
comment:14
This ticket is still needs_work. Remaining items include:
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. |
Author: Jason Grout |
comment:15
Also todo: set barchart and scatterplot to have default aspect ratios of 'auto'. |
comment:16
Replying to @jasongrout:
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. |
Attachment: 2100-aspect_ratio.2.patch.gz apply instead of previous patch |
comment:17
New patch revamping the aspect ratio calculations in Sage (and defaults!) |
comment:18
Updating description to reflect changes in the patch. |
Changed reviewer from Andrey Novoseltsev, Karl-Dieter Crisman to Andrey Novoseltsev, Karl-Dieter Crisman, Ryan Grout |
comment:68
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
Replying to @sagetrac-ryan:
What exactly do you mean by this? That a clean public release of Sage has doctest failures in |
comment:70
Correct - or to be more precise, warnings that do not cause the doctest to fail. They are known, and not from Sage. |
comment:72
Please rebase this patch to be applied on top of #11491. |
Work Issues: rebase |
comment:73
Replying to @jdemeyer:
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. |
Attachment: trac_2100-auto-automatic-and-vector.patch.gz Apply after rebase patch |
comment:74
Okay, now we should hopefully be at positive review! |
comment:75
Replying to @kcrisman:
I have done that in the past and people complained: http://groups.google.com/group/sage-devel/browse_frm/thread/abd5fb944769e052/8e4057172b97f2e5 |
comment:76
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
Replying to @kcrisman:
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. |
Changed work issues from rebase to none |
comment:79
Understood.
Yes, that definitely makes sense, as I hope I make clear above. :) |
Merged: sage-4.7.2.alpha0 |
We have two concepts of "aspect ratio" that we'd like to expose to the user.
This patch exposes the following behavior:
To patchbot/reviewers: Apply attachment: trac_2100-aspect-ratio-rebase.patch and attachment: trac_2100-auto-automatic-and-vector.patch
CC: @kcrisman @sagetrac-mhampton @qed777
Component: graphics
Keywords: sd31
Author: Jason Grout, Karl-Dieter Crisman
Reviewer: Andrey Novoseltsev, Karl-Dieter Crisman, Ryan Grout
Merged: sage-4.7.2.alpha0
Issue created by migration from https://trac.sagemath.org/ticket/2100
The text was updated successfully, but these errors were encountered: