Sage: Ticket #2100: sensible defaults for aspect ratio
https://trac.sagemath.org/ticket/2100
<p>
We have two concepts of "aspect ratio" that we'd like to expose to the user.
</p>
<p>
This patch exposes the following behavior:
</p>
<pre class="wiki"> * 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.
</pre><p>
Apply trac_2100-aspect-ratio-rebase.patch and trac_2100-auto-automatic-and-vector.patch for this.
</p>
en-usSagehttps://trac.sagemath.org/chrome/site/logo_sagemath_trac.png
https://trac.sagemath.org/ticket/2100
Trac 1.1.6jasonSun, 30 Mar 2008 05:03:22 GMT
https://trac.sagemath.org/ticket/2100#comment:1
https://trac.sagemath.org/ticket/2100#comment:1
<p>
For reference, in Mathematica (see <a class="ext-link" href="http://reference.wolfram.com/mathematica/ref/AspectRatio.html?q=AspectRatio&lang=en"><span class="icon"></span>http://reference.wolfram.com/mathematica/ref/AspectRatio.html?q=AspectRatio&lang=en</a> )
</p>
<ul><li>Graphics objects use Automatic aspect ratio
</li><li>plot and listplot use 1/golden ratio
</li><li>parametric plot uses automatic
</li><li>contour plot uses aspect ratio of 1
</li></ul><p>
Automatic is an aspect ratio determined by the pixels in the plot. It does not distort the plot (i.e., circles are still circles).
</p>
TicketmabshoffFri, 31 Oct 2008 05:41:57 GMT
https://trac.sagemath.org/ticket/2100#comment:2
https://trac.sagemath.org/ticket/2100#comment:2
<p>
Jason,
</p>
<p>
didn't we fix this recently?
</p>
<p>
Cheers,
</p>
<p>
Michael
</p>
TicketAlexGhitzaThu, 22 Jan 2009 18:28:03 GMTtype changed
https://trac.sagemath.org/ticket/2100#comment:3
https://trac.sagemath.org/ticket/2100#comment:3
<ul>
<li><strong>type</strong>
changed from <em>defect</em> to <em>enhancement</em>
</li>
</ul>
TicketjasonTue, 07 Sep 2010 03:34:21 GMTupstream set
https://trac.sagemath.org/ticket/2100#comment:4
https://trac.sagemath.org/ticket/2100#comment:4
<ul>
<li><strong>upstream</strong>
set to <em>N/A</em>
</li>
</ul>
<p>
<a class="closed ticket" href="https://trac.sagemath.org/ticket/9813" title="enhancement: parametric plot and polar plot aspect ratio (closed: fixed)">#9813</a> takes care of two cases of these.
</p>
<p>
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.
</p>
TicketjasonWed, 29 Sep 2010 22:00:12 GMT
https://trac.sagemath.org/ticket/2100#comment:5
https://trac.sagemath.org/ticket/2100#comment:5
<table class="wiki">
<tr><td> Matplotlib aspect ratio setting </td><td> Matplotlib adjustable setting </td><td> Mathematica <a class="ext-link" href="http://trac.sagemath.org/sage_trac/search/opensearch?q=wiki%3AAspectRatio"><span class="icon"></span>AspectRatio</a> setting </td><td> Explanation
</td></tr><tr><td> 'auto' </td><td> doesn't seem to matter </td><td> Full </td><td> Make the current data limits fill the available sized box
</td></tr><tr><td> 'equal' or 1 </td><td> 'box' </td><td> Automatic </td><td> Make each pixel be square (aspect ratio 1)
</td></tr><tr><td> </td><td> </td><td> </td><td>
</td></tr></table>
<p>
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.
</p>
<p>
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.
</p>
TicketjasonWed, 29 Sep 2010 22:03:27 GMT
https://trac.sagemath.org/ticket/2100#comment:6
https://trac.sagemath.org/ticket/2100#comment:6
<p>
Matplotlib's fig_aspect seems to do the sort of thing that mma's aspect ratio number setting does:
</p>
<p>
http://matplotlib.sourceforge.net/api/figure_api.html#matplotlib.figure.figaspect
</p>
TicketjasonWed, 29 Sep 2010 22:05:03 GMTcc set
https://trac.sagemath.org/ticket/2100#comment:7
https://trac.sagemath.org/ticket/2100#comment:7
<ul>
<li><strong>cc</strong>
<em>kcrisman</em> added
</li>
</ul>
TicketjasonWed, 29 Sep 2010 22:32:14 GMT
https://trac.sagemath.org/ticket/2100#comment:8
https://trac.sagemath.org/ticket/2100#comment:8
<p>
One other note: setting bbox_inches='tight' in the savefig method seems to override the figure size specified.
</p>
TicketjasonThu, 30 Sep 2010 02:46:58 GMT
https://trac.sagemath.org/ticket/2100#comment:9
https://trac.sagemath.org/ticket/2100#comment:9
<p>
We have two concepts of "aspect ratio" that we'd like to expose to the user.
</p>
<ul><li>Figure aspect ratio -- controls the final image size, including any text labels, etc. This will be set using the following options:
<ul><li>fig_aspect_ratio
<ul><li>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...)
</li><li>'auto' (default) - use bbox_inches='tight' to create a figure that holds the drawn objects
</li></ul></li><li>figsize
<ul><li>single number - use this as a base size for fig_aspect_ratio calculations (i.e., bigger numbers = bigger figures)
</li><li>two numbers - an actual figure size in inches
</li></ul></li></ul></li><li>Pixel aspect ratio
<ul><li>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
<ul><li>'equal' or 1 -- square pixels. This will be the default for most things
</li><li>'auto' -- plot the given data limits in the given (or computed) figsize, filling the figure
</li><li>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
</li></ul></li><li>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
<ul><li>'box' (default) -- the frame axes
</li><li>'datalim' -- the data limits
</li></ul></li></ul></li></ul><p>
What do people think?
</p>
TicketjasonThu, 30 Sep 2010 03:09:38 GMT
https://trac.sagemath.org/ticket/2100#comment:10
https://trac.sagemath.org/ticket/2100#comment:10
<p>
Helpful matplotlib mailing list post (read the thread): http://www.mail-archive.com/matplotlib-users@lists.sourceforge.net/msg15623.html
</p>
TicketjasonThu, 30 Sep 2010 14:23:09 GMTdescription changed
https://trac.sagemath.org/ticket/2100#comment:11
https://trac.sagemath.org/ticket/2100#comment:11
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/2100?action=diff&version=11">diff</a>)
</li>
</ul>
<p>
Moving the proposal up to the description so we can edit it. Ignore the proposal in the comments now.
</p>
TicketjasonFri, 01 Oct 2010 02:01:28 GMTdescription changed
https://trac.sagemath.org/ticket/2100#comment:12
https://trac.sagemath.org/ticket/2100#comment:12
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/2100?action=diff&version=12">diff</a>)
</li>
</ul>
<p>
Update the proposal.
</p>
TicketjasonFri, 01 Oct 2010 02:07:06 GMT
https://trac.sagemath.org/ticket/2100#comment:13
https://trac.sagemath.org/ticket/2100#comment:13
<p>
I've attached two files. First, a work-in-progress rewrite of the aspect ratio stuff. Second, the plot.py file before this patch was applied. The plot.py file is the version from 4.6.alpha1 with tickets <a class="ext-link" href="http://trac.sagemath.org/sage_trac/search/opensearch?q=ticket%3A9740"><span class="icon"></span>#9740</a>, <a class="ext-link" href="http://trac.sagemath.org/sage_trac/search/opensearch?q=ticket%3A9746"><span class="icon"></span>#9746</a>, and <a class="ext-link" href="http://trac.sagemath.org/sage_trac/search/opensearch?q=ticket%3A4342"><span class="icon"></span>#4342</a> applied, in order.
</p>
TicketjasonFri, 01 Oct 2010 02:11:15 GMTstatus changed; author set
https://trac.sagemath.org/ticket/2100#comment:14
https://trac.sagemath.org/ticket/2100#comment:14
<ul>
<li><strong>status</strong>
changed from <em>new</em> to <em>needs_work</em>
</li>
<li><strong>author</strong>
set to <em>Jason Grout</em>
</li>
</ul>
<p>
This ticket is still needs_work. Remaining items include:
</p>
<ul><li>Updating docs to reflect changes, including
<ul><li>Most aspect ratios now default to 1
</li><li>new/changed options:
<ul><li> fig_tight
</li><li>the new definition of aspect_ratio (= 1/old definition)
</li><li>aspect_ratio_adjustable
</li></ul></li></ul></li><li>Make <a class="ext-link" href="http://trac.sagemath.org/sage_trac/search/opensearch?q=wiki%3AGraphicsArray"><span class="icon"></span>GraphicsArray</a> work (likely using the nice features of matplotlib in 1.0 for doing multiple subplots!)
</li></ul><p>
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.
</p>
<p>
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.
</p>
TicketjasonFri, 01 Oct 2010 03:31:57 GMT
https://trac.sagemath.org/ticket/2100#comment:15
https://trac.sagemath.org/ticket/2100#comment:15
<p>
Also todo: set barchart and scatterplot to have default aspect ratios of 'auto'.
</p>
TicketjasonSat, 02 Oct 2010 02:29:43 GMT
https://trac.sagemath.org/ticket/2100#comment:16
https://trac.sagemath.org/ticket/2100#comment:16
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/2100#comment:14" title="Comment 14">jason</a>:
</p>
<blockquote class="citation">
<p>
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.
</p>
</blockquote>
<p>
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.
</p>
<p>
So, in short, things are good to go for finishing this patch and getting it into Sage as soon as possible.
</p>
TicketjasonThu, 07 Oct 2010 07:08:23 GMTattachment set
https://trac.sagemath.org/ticket/2100
https://trac.sagemath.org/ticket/2100
<ul>
<li><strong>attachment</strong>
set to <em>2100-aspect_ratio.2.patch</em>
</li>
</ul>
<p>
apply instead of previous patch
</p>
TicketjasonThu, 07 Oct 2010 07:10:20 GMTstatus changed
https://trac.sagemath.org/ticket/2100#comment:17
https://trac.sagemath.org/ticket/2100#comment:17
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
</ul>
<p>
New patch revamping the aspect ratio calculations in Sage (and defaults!)
</p>
TicketjasonThu, 07 Oct 2010 16:37:57 GMTdescription changed
https://trac.sagemath.org/ticket/2100#comment:18
https://trac.sagemath.org/ticket/2100#comment:18
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/2100?action=diff&version=18">diff</a>)
</li>
</ul>
<p>
Updating description to reflect changes in the patch.
</p>
TicketnovoseltSun, 21 Nov 2010 18:15:51 GMT
https://trac.sagemath.org/ticket/2100#comment:19
https://trac.sagemath.org/ticket/2100#comment:19
<p>
It seems to me that <code>set_aspect</code> ratio only accepts "auto" as a string input, while the proposal also lists "equal" as an option.
</p>
<p>
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 <code>width=\textwidth</code> or <code>height=0.5\texthight</code> and have the second dimension determined automatically based on the actual plot and aspect ratio.
</p>
TicketnovoseltSun, 21 Nov 2010 18:24:06 GMTstatus changed; work_issues set
https://trac.sagemath.org/ticket/2100#comment:20
https://trac.sagemath.org/ticket/2100#comment:20
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
<li><strong>work_issues</strong>
set to <em>complete documentation</em>
</li>
</ul>
<p>
New functions <code>pad_for_tick_labels</code> and <code>adjust_figure_to_contain_bbox</code> are not completely documented.
</p>
<p>
Are <a class="closed ticket" href="https://trac.sagemath.org/ticket/9740" title="enhancement: matrix plot is upside down and should wrap more matplotlib options (closed: fixed)">#9740</a>, <a class="closed ticket" href="https://trac.sagemath.org/ticket/9746" title="enhancement: documentation for plotting (closed: fixed)">#9746</a>, and <a class="closed ticket" href="https://trac.sagemath.org/ticket/4342" title="enhancement: Add legends to plot.py (closed: fixed)">#4342</a> still prerequisites?
</p>
TicketnovoseltSun, 21 Nov 2010 18:25:05 GMT
https://trac.sagemath.org/ticket/2100#comment:21
https://trac.sagemath.org/ticket/2100#comment:21
<p>
Oops, got the answer to the last question already, for some reason they were not crossed in the above comment.
</p>
TicketjasonWed, 12 Jan 2011 21:43:56 GMTcc changed
https://trac.sagemath.org/ticket/2100#comment:22
https://trac.sagemath.org/ticket/2100#comment:22
<ul>
<li><strong>cc</strong>
<em>mhampton</em> added
</li>
</ul>
<p>
This might be good bug-day fodder if people are already working on graphics code.
</p>
TicketkcrismanFri, 14 Jan 2011 13:26:08 GMTreviewer set
https://trac.sagemath.org/ticket/2100#comment:23
https://trac.sagemath.org/ticket/2100#comment:23
<ul>
<li><strong>reviewer</strong>
set to <em>Andrey Novoseltsev</em>
</li>
</ul>
<p>
I hope to take care of novoselt's comments - 'equal' and the documentation - today. This will depend on <a class="closed ticket" href="https://trac.sagemath.org/ticket/7981" title="defect: animate ignores options to show that are passed up from the plot command (closed: fixed)">#7981</a>, <a class="closed ticket" href="https://trac.sagemath.org/ticket/8632" title="defect: .save ignores ymin etc. (closed: fixed)">#8632</a>, <a class="closed ticket" href="https://trac.sagemath.org/ticket/10244" title="defect: legends don't save correctly (closed: fixed)">#10244</a>, and <a class="closed ticket" href="https://trac.sagemath.org/ticket/10143" title="enhancement: Bring 2D plotting up to 100% doctest coverage (except plot.py) (closed: fixed)">#10143</a>.
</p>
<blockquote class="citation">
<p>
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 <code>width=\textwidth</code> or <code>height=0.5\texthight</code> and have the second dimension determined automatically based on the actual plot and aspect ratio.
</p>
</blockquote>
<p>
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 <a class="new ticket" href="https://trac.sagemath.org/ticket/10633" title="enhancement: Allow only width or height of figures to be specified (new)">#10633</a> for this.
</p>
TicketkcrismanFri, 14 Jan 2011 13:28:45 GMT
https://trac.sagemath.org/ticket/2100#comment:24
https://trac.sagemath.org/ticket/2100#comment:24
<p>
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.
</p>
TicketjasonFri, 14 Jan 2011 16:06:29 GMT
https://trac.sagemath.org/ticket/2100#comment:25
https://trac.sagemath.org/ticket/2100#comment:25
<p>
Let's just take the 'equal' option out of the proposal. I think it's there just because that's what matplotlib does.
</p>
TicketkcrismanFri, 14 Jan 2011 16:09:12 GMT
https://trac.sagemath.org/ticket/2100#comment:26
https://trac.sagemath.org/ticket/2100#comment:26
<p>
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().
</p>
<p>
I can also remove 'equal' if you want, that's fine.
</p>
<p>
Before I go ahead and update this, though, I have a question. The two functions not documented (<code>pad_for_tick_labels</code> and <code>adjust_figure_to_contain_bbox</code>) only are used in a commented-out line
</p>
<pre class="wiki"> # this messes up the aspect ratio!
#figure.canvas.mpl_connect('draw_event', pad_for_tick_labels)
</pre><p>
So... should they even be included at this time?
</p>
TicketkcrismanFri, 14 Jan 2011 16:10:12 GMTdescription changed
https://trac.sagemath.org/ticket/2100#comment:27
https://trac.sagemath.org/ticket/2100#comment:27
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/2100?action=diff&version=27">diff</a>)
</li>
</ul>
TicketkcrismanMon, 17 Jan 2011 17:25:15 GMT
https://trac.sagemath.org/ticket/2100#comment:28
https://trac.sagemath.org/ticket/2100#comment:28
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/2100#comment:26" title="Comment 26">kcrisman</a>:
</p>
<blockquote class="citation">
<p>
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().
</p>
<p>
I can also remove 'equal' if you want, that's fine.
</p>
</blockquote>
<p>
Done as well.
</p>
<blockquote class="citation">
<p>
Before I go ahead and update this, though, I have a question. The two functions not documented (<code>pad_for_tick_labels</code> and <code>adjust_figure_to_contain_bbox</code>) only are used in a commented-out line
</p>
<pre class="wiki"> # this messes up the aspect ratio!
#figure.canvas.mpl_connect('draw_event', pad_for_tick_labels)
</pre><p>
So... should they even be included at this time?
</p>
</blockquote>
<p>
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)."
</p>
<p>
New patch coming up relatively soon.
</p>
TicketkcrismanMon, 17 Jan 2011 19:03:27 GMT
https://trac.sagemath.org/ticket/2100#comment:29
https://trac.sagemath.org/ticket/2100#comment:29
<blockquote class="citation">
<blockquote class="citation">
<p>
So... should they even be included at this time?
</p>
</blockquote>
<p>
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)."
</p>
</blockquote>
<p>
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.)
</p>
<p>
Getting some doctest failures, though, related to this:
</p>
<pre class="wiki">sage -t devel/sage/sage/plot/plot3d/plot_field3d.py # 6 doctests failed
</pre><p>
I doubt this will be hard to resolve. Anyway, just status updates...
</p>
TicketkcrismanMon, 17 Jan 2011 20:06:30 GMTattachment set
https://trac.sagemath.org/ticket/2100
https://trac.sagemath.org/ticket/2100
<ul>
<li><strong>attachment</strong>
set to <em>trac_2100-aspect-ratio-rebase.patch</em>
</li>
</ul>
<p>
Rebase to 4.6.1/4.6.2.alpha0
</p>
TicketkcrismanMon, 17 Jan 2011 20:17:54 GMTstatus, reviewer, author changed
https://trac.sagemath.org/ticket/2100#comment:30
https://trac.sagemath.org/ticket/2100#comment:30
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
<li><strong>reviewer</strong>
changed from <em>Andrey Novoseltsev</em> to <em>Andrey Novoseltsev, Karl-Dieter Crisman</em>
</li>
<li><strong>author</strong>
changed from <em>Jason Grout</em> to <em>Jason Grout, Karl-Dieter Crisman</em>
</li>
</ul>
<p>
The problem with the <code>plot_field3d</code> turns out to be the following very bad behavior of plotting vectors after this patch
</p>
<pre class="wiki">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
</pre><p>
The culprit was telling <code>plot_vector_field3d</code> to do <code>plot(v)</code> for each vector instead of <code>v.plot()</code> as it should have. However, this exposes something else really dumb - namely, that <code>plot.py</code> turns vectors into tuples before it plots them
</p>
<pre class="wiki"> 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:
</pre><p>
this presumably avoids the fact that
</p>
<pre class="wiki">sage: w = vector([x^2,3,x^3])
sage: plot(w,(x,0,1))
---------------------------------------------------------------------------
NotImplementedError
</pre><p>
but still this is a problem. So after fixing this issue, I'm opening a ticket for making vector plotting more sensible.
</p>
<p>
I think this change might need review (?) so I'm setting to 'needs review'.
</p>
<p>
To buildbot and reviewer: apply trac_2100-aspect-ratio-rebase.patch and trac_2100-3d-vector.patch. Depends on <a class="closed ticket" href="https://trac.sagemath.org/ticket/7981" title="defect: animate ignores options to show that are passed up from the plot command (closed: fixed)">#7981</a>, <a class="closed ticket" href="https://trac.sagemath.org/ticket/8632" title="defect: .save ignores ymin etc. (closed: fixed)">#8632</a>, <a class="closed ticket" href="https://trac.sagemath.org/ticket/10244" title="defect: legends don't save correctly (closed: fixed)">#10244</a>, and <a class="closed ticket" href="https://trac.sagemath.org/ticket/10143" title="enhancement: Bring 2D plotting up to 100% doctest coverage (except plot.py) (closed: fixed)">#10143</a>.
</p>
TicketkcrismanMon, 17 Jan 2011 20:28:01 GMTwork_issues deleted
https://trac.sagemath.org/ticket/2100#comment:31
https://trac.sagemath.org/ticket/2100#comment:31
<ul>
<li><strong>work_issues</strong>
<em>complete documentation</em> deleted
</li>
</ul>
TicketkcrismanTue, 18 Jan 2011 15:45:37 GMT
https://trac.sagemath.org/ticket/2100#comment:32
https://trac.sagemath.org/ticket/2100#comment:32
<blockquote class="citation">
<p>
I think this change might need review (?) so I'm setting to 'needs review'.
</p>
</blockquote>
<p>
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.
</p>
TicketnovoseltTue, 18 Jan 2011 22:53:00 GMT
https://trac.sagemath.org/ticket/2100#comment:33
https://trac.sagemath.org/ticket/2100#comment:33
<p>
Depends on <a class="closed ticket" href="https://trac.sagemath.org/ticket/10143" title="enhancement: Bring 2D plotting up to 100% doctest coverage (except plot.py) (closed: fixed)">#10143</a>
</p>
<p>
(others are "inherited")
</p>
TicketnovoseltWed, 19 Jan 2011 03:45:25 GMTstatus changed
https://trac.sagemath.org/ticket/2100#comment:34
https://trac.sagemath.org/ticket/2100#comment:34
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>positive_review</em>
</li>
</ul>
<p>
I'm setting this to positive review - it is a great improvement and it is a shame that it took us so long ;-)
</p>
<p>
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!!!
</p>
<p>
I have also discovered the following behaviour which may be a bug:
</p>
<pre class="wiki">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)
</pre><p>
The first line (with <code>fill=True</code>) completely fills the plot, not just the disk.
</p>
TicketkcrismanWed, 19 Jan 2011 03:51:59 GMT
https://trac.sagemath.org/ticket/2100#comment:35
https://trac.sagemath.org/ticket/2100#comment:35
<p>
Thanks!
</p>
<blockquote class="citation">
<p>
I have also discovered the following behaviour which may be a bug:
</p>
<pre class="wiki">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)
</pre><p>
The first line (with <code>fill=True</code>) completely fills the plot, not just the disk.
</p>
</blockquote>
<p>
This also happens with e.g. 4.6.1.alpha3, so this is definitely not related. It sounds a LOT like <a class="closed ticket" href="https://trac.sagemath.org/ticket/9744" title="defect: implicit_plot fill option fills entire plot (closed: fixed)">#9744</a>. It'd be great if you could confirm that. I thought we'd already taken care of this... unfortunately not, it seems.
</p>
TicketnovoseltWed, 19 Jan 2011 04:01:36 GMTstatus changed; work_issues set
https://trac.sagemath.org/ticket/2100#comment:36
https://trac.sagemath.org/ticket/2100#comment:36
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>needs_work</em>
</li>
<li><strong>work_issues</strong>
set to <em>Plotting of vectors examples</em>
</li>
</ul>
<p>
Actually, I was too hasty:
</p>
<pre class="wiki">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.
</pre><p>
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?..
</p>
TicketnovoseltWed, 19 Jan 2011 04:29:05 GMT
https://trac.sagemath.org/ticket/2100#comment:37
https://trac.sagemath.org/ticket/2100#comment:37
<p>
Yes, the problem I hit is exactly <a class="closed ticket" href="https://trac.sagemath.org/ticket/9744" title="defect: implicit_plot fill option fills entire plot (closed: fixed)">#9744</a> (which actually mentions the same example from the documentation), thanks for pointing it out.
</p>
TicketkcrismanWed, 19 Jan 2011 13:30:15 GMT
https://trac.sagemath.org/ticket/2100#comment:38
https://trac.sagemath.org/ticket/2100#comment:38
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/2100#comment:36" title="Comment 36">novoselt</a>:
</p>
<blockquote class="citation">
<p>
Actually, I was too hasty:
sage -t -long "devel/sage-main/sage/modules/free_module_element.pyx"
</p>
</blockquote>
<p>
Aargh! I'll try to fix this...
</p>
<blockquote class="citation">
<p>
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?..
</p>
</blockquote>
<p>
No, that is the correct fix for now. An overarching ticket is <a class="new ticket" href="https://trac.sagemath.org/ticket/10638" title="enhancement: Plot vectors of expressions without using parametric (new)">#10638</a>, which I forgot to mention before.
</p>
TicketkcrismanWed, 19 Jan 2011 13:35:18 GMT
https://trac.sagemath.org/ticket/2100#comment:39
https://trac.sagemath.org/ticket/2100#comment:39
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/2100#comment:38" title="Comment 38">kcrisman</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/2100#comment:36" title="Comment 36">novoselt</a>:
</p>
<blockquote class="citation">
<p>
Actually, I was too hasty:
sage -t -long "devel/sage-main/sage/modules/free_module_element.pyx"
</p>
</blockquote>
<p>
Aargh! I'll try to fix this...
</p>
<blockquote class="citation">
<p>
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?..
</p>
</blockquote>
<p>
No, that is the correct fix for now. An overarching ticket is <a class="new ticket" href="https://trac.sagemath.org/ticket/10638" title="enhancement: Plot vectors of expressions without using parametric (new)">#10638</a>, which I forgot to mention before.
</p>
</blockquote>
<p>
The correct fix is to remove that kwd from the plot when
</p>
<pre class="wiki">
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)
</pre><p>
that should do it. I will try this later.
</p>
TicketkcrismanWed, 19 Jan 2011 19:24:36 GMT
https://trac.sagemath.org/ticket/2100#comment:40
https://trac.sagemath.org/ticket/2100#comment:40
<blockquote class="citation">
<blockquote class="citation">
<blockquote class="citation">
<p>
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?..
</p>
</blockquote>
<p>
No, that is the correct fix for now. An overarching ticket is <a class="new ticket" href="https://trac.sagemath.org/ticket/10638" title="enhancement: Plot vectors of expressions without using parametric (new)">#10638</a>, which I forgot to mention before.
</p>
</blockquote>
</blockquote>
<p>
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 <a class="new ticket" href="https://trac.sagemath.org/ticket/10638" title="enhancement: Plot vectors of expressions without using parametric (new)">#10638</a>.
</p>
<p>
I <em>think</em> the issue is that <code>_extract_kwds_for_show</code> keeps the <code>@options</code> value of <code>aspect_ratio='auto') in play; when this gets passed to the </code>show()<code> method of </code>Line<code> (which is what </code>line3d<code> creates in this case), it refers to the one in </code>base.<a class="missing wiki">PrimitiveObject?</a><code>, which unfortunately then puts this value of </code>aspect_ratio` in:
</p>
<pre class="wiki"> opts = {}
opts.update(SHOW_DEFAULTS)
if self._extra_kwds is not None:
opts.update(self._extra_kwds)
opts.update(kwds)
</pre><p>
and then doesn't realize it's a problem in
</p>
<pre class="wiki"> 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'])
</pre><p>
Which is where the problem happens.
</p>
<p>
Although fixing this in the vector plotting didn't seem to work immediately.
</p>
<p>
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?
</p>
TicketnovoseltWed, 19 Jan 2011 19:47:05 GMT
https://trac.sagemath.org/ticket/2100#comment:41
https://trac.sagemath.org/ticket/2100#comment:41
<p>
I like <code>auto</code> more than <code>automatic</code>, so I don't want to change it.
</p>
<p>
Can we maybe change it the other way? Was <code>automatic</code> documented/used somewhere before?
</p>
TicketkcrismanWed, 19 Jan 2011 20:10:19 GMT
https://trac.sagemath.org/ticket/2100#comment:42
https://trac.sagemath.org/ticket/2100#comment:42
<blockquote class="citation">
<p>
I like <code>auto</code> more than <code>automatic</code>, so I don't want to change it.
</p>
</blockquote>
<p>
Agreed, but...
</p>
<blockquote class="citation">
<p>
Can we maybe change it the other way? Was <code>automatic</code> documented/used somewhere before?
</p>
</blockquote>
<p>
Yes, this is the standard 3D option. See <a href="http://www.sagemath.org/doc/reference/sage/plot/plot3d/base.html#sage.plot.plot3d.base.Graphics3d.show">this reference manual page</a> for plot/plot3d/base.pyx.
</p>
<p>
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.
</p>
TicketnovoseltWed, 19 Jan 2011 20:16:16 GMT
https://trac.sagemath.org/ticket/2100#comment:43
https://trac.sagemath.org/ticket/2100#comment:43
<p>
I guess then changing to <code>automatic</code> is OK.
</p>
<p>
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.
</p>
TicketkcrismanWed, 19 Jan 2011 20:21:13 GMT
https://trac.sagemath.org/ticket/2100#comment:44
https://trac.sagemath.org/ticket/2100#comment:44
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/2100#comment:43" title="Comment 43">novoselt</a>:
</p>
<blockquote class="citation">
<p>
I guess then changing to <code>automatic</code> is OK.
</p>
</blockquote>
<p>
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).
</p>
<blockquote class="citation">
<p>
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.
</p>
</blockquote>
TicketkcrismanWed, 19 Jan 2011 20:32:26 GMT
https://trac.sagemath.org/ticket/2100#comment:45
https://trac.sagemath.org/ticket/2100#comment:45
<blockquote class="citation">
<blockquote class="citation">
<p>
I guess then changing to <code>automatic</code> is OK.
</p>
</blockquote>
<p>
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).
</p>
</blockquote>
<p>
One reason being that matplotlib uses 'auto', and consistency with that is nice as well.
</p>
<blockquote class="citation">
<blockquote class="citation">
<p>
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.
</p>
</blockquote>
</blockquote>
TicketkcrismanWed, 19 Jan 2011 20:36:07 GMTstatus changed
https://trac.sagemath.org/ticket/2100#comment:46
https://trac.sagemath.org/ticket/2100#comment:46
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
</ul>
<p>
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.
</p>
TicketkcrismanWed, 19 Jan 2011 20:38:49 GMTattachment set
https://trac.sagemath.org/ticket/2100
https://trac.sagemath.org/ticket/2100
<ul>
<li><strong>attachment</strong>
set to <em>trac_2100-3d-vector.patch</em>
</li>
</ul>
<p>
reviewer patch
</p>
TicketkcrismanWed, 19 Jan 2011 20:39:26 GMTwork_issues deleted
https://trac.sagemath.org/ticket/2100#comment:47
https://trac.sagemath.org/ticket/2100#comment:47
<ul>
<li><strong>work_issues</strong>
<em>Plotting of vectors examples</em> deleted
</li>
</ul>
TicketjasonWed, 19 Jan 2011 21:24:13 GMT
https://trac.sagemath.org/ticket/2100#comment:48
https://trac.sagemath.org/ticket/2100#comment:48
<p>
Wow, great job tracking down this subtle problem. I personally slightly prefer <code>'auto'</code> over <code>'automatic'</code> because it's consistent with matplotlib and it's shorter. However, I agree that users will probably not specify it too frequently, so <code>'automatic'</code> would probably be fine as well. If we do make the default <code>'automatic'</code>, then I think we'll have to convert it to <code>'auto'</code> before passing it to matplotlib.
</p>
<p>
Another data point: mma uses Automatic (not Auto): <a class="ext-link" href="http://reference.wolfram.com/mathematica/ref/Automatic.html"><span class="icon"></span>http://reference.wolfram.com/mathematica/ref/Automatic.html</a>
</p>
TicketkcrismanWed, 19 Jan 2011 21:31:43 GMT
https://trac.sagemath.org/ticket/2100#comment:49
https://trac.sagemath.org/ticket/2100#comment:49
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/2100#comment:48" title="Comment 48">jason</a>:
</p>
<blockquote class="citation">
<p>
Wow, great job tracking down this subtle problem. I personally slightly prefer <code>'auto'</code> over <code>'automatic'</code> because it's consistent with matplotlib and it's shorter. However, I agree that users will probably not specify it too frequently, so <code>'automatic'</code> would probably be fine as well.
</p>
</blockquote>
<p>
Thanks. I don't have any more time today to work on this, but I think then I vote for <code>'automatic'</code> for consistency, and having <code>'auto'</code> as one that secretly also works :)
</p>
<blockquote class="citation">
<p>
If we do make the default <code>'automatic'</code>, then I think we'll have to convert it to <code>'auto'</code> before passing it to matplotlib.
</p>
</blockquote>
<p>
Yes, I realize that. I don't think that will be very difficult, since there seems to be only one place this is passed.
</p>
TicketkcrismanFri, 21 Jan 2011 13:28:06 GMTstatus changed; work_issues set
https://trac.sagemath.org/ticket/2100#comment:50
https://trac.sagemath.org/ticket/2100#comment:50
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
<li><strong>work_issues</strong>
set to <em>unify option names</em>
</li>
</ul>
<p>
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.
</p>
TicketkcrismanTue, 08 Feb 2011 18:01:18 GMT
https://trac.sagemath.org/ticket/2100#comment:51
https://trac.sagemath.org/ticket/2100#comment:51
<p>
Oops... hmm, I gave myself one long weekend. I'll try to see if this still applies to 4.6.2.alpha4 soon.
</p>
TicketkcrismanMon, 14 Feb 2011 19:47:27 GMT
https://trac.sagemath.org/ticket/2100#comment:52
https://trac.sagemath.org/ticket/2100#comment:52
<p>
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.
</p>
TicketkcrismanTue, 15 Feb 2011 03:01:20 GMTstatus, description changed
https://trac.sagemath.org/ticket/2100#comment:53
https://trac.sagemath.org/ticket/2100#comment:53
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
<li><strong>description</strong>
modified (<a href="/ticket/2100?action=diff&version=53">diff</a>)
</li>
</ul>
<p>
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!
</p>
TicketjasonTue, 15 Feb 2011 03:08:24 GMT
https://trac.sagemath.org/ticket/2100#comment:54
https://trac.sagemath.org/ticket/2100#comment:54
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/2100#comment:53" title="Comment 53">kcrisman</a>:
</p>
<blockquote class="citation">
<p>
This should all work fine now. I do want to point out that the standard plots seem to have increased somewhat in size.
</p>
</blockquote>
<p>
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).
</p>
TicketjasonTue, 15 Feb 2011 03:09:25 GMTowner changed
https://trac.sagemath.org/ticket/2100#comment:55
https://trac.sagemath.org/ticket/2100#comment:55
<ul>
<li><strong>owner</strong>
changed from <em>was</em> to <em>jason</em>
</li>
</ul>
<p>
Can you add instructions for reviewers/patchbot about which patches should be applied in what order?
</p>
TicketjasonTue, 15 Feb 2011 03:09:59 GMT
https://trac.sagemath.org/ticket/2100#comment:56
https://trac.sagemath.org/ticket/2100#comment:56
<p>
(and then I'll try to review this tonight or tomorrow)
</p>
TicketkcrismanTue, 15 Feb 2011 03:11:22 GMTdescription changed
https://trac.sagemath.org/ticket/2100#comment:57
https://trac.sagemath.org/ticket/2100#comment:57
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/2100?action=diff&version=57">diff</a>)
</li>
</ul>
<p>
See the description. I guess I didn't make it obvious enough :)
</p>
<p>
Apply [trac_2100-aspect-ratio-rebase.patch] and [trac_2100-auto-automatic-and-vector.patch] for this.
</p>
TicketkcrismanTue, 15 Feb 2011 03:12:12 GMTdescription changed
https://trac.sagemath.org/ticket/2100#comment:58
https://trac.sagemath.org/ticket/2100#comment:58
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/2100?action=diff&version=58">diff</a>)
</li>
</ul>
<p>
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.
</p>
TicketkcrismanTue, 15 Feb 2011 03:12:31 GMT
https://trac.sagemath.org/ticket/2100#comment:59
https://trac.sagemath.org/ticket/2100#comment:59
<p>
I can't get the links to work for some reason - sorry.
</p>
TicketkcrismanWed, 16 Feb 2011 17:17:43 GMTdescription changed
https://trac.sagemath.org/ticket/2100#comment:60
https://trac.sagemath.org/ticket/2100#comment:60
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/2100?action=diff&version=60">diff</a>)
</li>
</ul>
<p>
To patchbot/reviewers: Apply <a class="attachment" href="https://trac.sagemath.org/attachment/ticket/2100/trac_2100-aspect-ratio-rebase.patch" title="Attachment 'trac_2100-aspect-ratio-rebase.patch' in Ticket #2100">attachment:trac_2100-aspect-ratio-rebase.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/2100/trac_2100-aspect-ratio-rebase.patch" title="Download"></a> and <a class="attachment" href="https://trac.sagemath.org/attachment/ticket/2100/trac_2100-auto-automatic-and-vector.patch" title="Attachment 'trac_2100-auto-automatic-and-vector.patch' in Ticket #2100">attachment:trac_2100-auto-automatic-and-vector.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/2100/trac_2100-auto-automatic-and-vector.patch" title="Download"></a>
</p>
TicketkcrismanSat, 19 Feb 2011 04:20:10 GMT
https://trac.sagemath.org/ticket/2100#comment:61
https://trac.sagemath.org/ticket/2100#comment:61
<p>
Here is some code that looks less good with this patch than before.
</p>
<pre class="wiki">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)
</pre><p>
I'll try to attach before and after.
</p>
TicketkcrismanSat, 19 Feb 2011 04:21:35 GMTattachment set
https://trac.sagemath.org/ticket/2100
https://trac.sagemath.org/ticket/2100
<ul>
<li><strong>attachment</strong>
set to <em>Before.png</em>
</li>
</ul>
<p>
What this plot looks like before <a class="closed ticket" href="https://trac.sagemath.org/ticket/2100" title="enhancement: sensible defaults for aspect ratio (closed: fixed)">#2100</a>
</p>
TicketkcrismanSat, 19 Feb 2011 04:21:54 GMTattachment set
https://trac.sagemath.org/ticket/2100
https://trac.sagemath.org/ticket/2100
<ul>
<li><strong>attachment</strong>
set to <em>After.png</em>
</li>
</ul>
<p>
What this plot looks like after <a class="closed ticket" href="https://trac.sagemath.org/ticket/2100" title="enhancement: sensible defaults for aspect ratio (closed: fixed)">#2100</a>
</p>
TicketkcrismanSat, 19 Feb 2011 04:24:04 GMTstatus changed
https://trac.sagemath.org/ticket/2100#comment:62
https://trac.sagemath.org/ticket/2100#comment:62
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_info</em>
</li>
</ul>
<p>
The issue is of course that I never used <code>plot()</code> to do this, but rather only <code>line()</code> and <code>point()</code> and <code>Graphics()</code>. 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?
</p>
<p>
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!
</p>
TicketjasonSat, 19 Feb 2011 04:54:43 GMT
https://trac.sagemath.org/ticket/2100#comment:63
https://trac.sagemath.org/ticket/2100#comment:63
<p>
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).
</p>
<p>
plot() defaults to 'auto' aspect ratio (e.g., fill the figure). However, geometric figures like line and point default to aspect_ratio=1.
</p>
TicketkcrismanSat, 05 Mar 2011 02:52:15 GMT
https://trac.sagemath.org/ticket/2100#comment:64
https://trac.sagemath.org/ticket/2100#comment:64
<p>
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.
</p>
<p>
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?
</p>
TicketkcrismanSat, 05 Mar 2011 02:53:01 GMTstatus changed; work_issues deleted
https://trac.sagemath.org/ticket/2100#comment:65
https://trac.sagemath.org/ticket/2100#comment:65
<ul>
<li><strong>status</strong>
changed from <em>needs_info</em> to <em>needs_review</em>
</li>
<li><strong>work_issues</strong>
<em>unify option names</em> deleted
</li>
</ul>
<p>
Putting needs review, though may need a minor change because of the thing in the last two comments.
</p>
TicketkcrismanWed, 15 Jun 2011 19:34:36 GMTkeywords set
https://trac.sagemath.org/ticket/2100#comment:66
https://trac.sagemath.org/ticket/2100#comment:66
<ul>
<li><strong>keywords</strong>
<em>sd31</em> added
</li>
</ul>
<p>
Okay, I'm going to refresh so that line and point still have 'auto'. Needs review!
</p>
TicketryanThu, 16 Jun 2011 22:46:49 GMTreviewer changed
https://trac.sagemath.org/ticket/2100#comment:67
https://trac.sagemath.org/ticket/2100#comment:67
<ul>
<li><strong>reviewer</strong>
changed from <em>Andrey Novoseltsev, Karl-Dieter Crisman</em> to <em>Andrey Novoseltsev, Karl-Dieter Crisman, Ryan Grout</em>
</li>
</ul>
<p>
FYI, I am currently reviewing this patch
</p>
TicketryanThu, 16 Jun 2011 23:47:35 GMTstatus changed
https://trac.sagemath.org/ticket/2100#comment:68
https://trac.sagemath.org/ticket/2100#comment:68
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>positive_review</em>
</li>
</ul>
<p>
Patch looks good. Confirmed that kcrisman's code looks 'good' after the patch too :) Positive Review. Great job guys.
</p>
<p>
Note: I do get doctest failures on plot_field.py, but the failures are not caused by the patches to this ticket.
</p>
TicketnovoseltThu, 16 Jun 2011 23:51:30 GMT
https://trac.sagemath.org/ticket/2100#comment:69
https://trac.sagemath.org/ticket/2100#comment:69
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/2100#comment:68" title="Comment 68">ryan</a>:
</p>
<blockquote class="citation">
<p>
Note: I do get doctest failures on plot_field.py, but the failures are not caused by the patches to this ticket.
</p>
</blockquote>
<p>
What exactly do you mean by this? That a clean public release of Sage has doctest failures in <code>plot_field.py</code>?
</p>
TicketkcrismanFri, 17 Jun 2011 00:13:02 GMT
https://trac.sagemath.org/ticket/2100#comment:70
https://trac.sagemath.org/ticket/2100#comment:70
<p>
Correct - or to be more precise, warnings that do not cause the doctest to fail. They are known, and not from Sage.
</p>
TicketjdemeyerSat, 18 Jun 2011 09:38:25 GMTmilestone changed
https://trac.sagemath.org/ticket/2100#comment:71
https://trac.sagemath.org/ticket/2100#comment:71
<ul>
<li><strong>milestone</strong>
changed from <em>sage-4.7.1</em> to <em>sage-4.7.2</em>
</li>
</ul>
TicketjdemeyerSat, 18 Jun 2011 10:11:54 GMTstatus changed; work_issues set
https://trac.sagemath.org/ticket/2100#comment:72
https://trac.sagemath.org/ticket/2100#comment:72
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>needs_work</em>
</li>
<li><strong>work_issues</strong>
set to <em>rebase</em>
</li>
</ul>
<p>
Please rebase this patch to be applied on top of <a class="closed ticket" href="https://trac.sagemath.org/ticket/11491" title="defect: Minor doc formatting problem in tachyon_repr (closed: fixed)">#11491</a>.
</p>
TicketkcrismanMon, 20 Jun 2011 14:45:19 GMT
https://trac.sagemath.org/ticket/2100#comment:73
https://trac.sagemath.org/ticket/2100#comment:73
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/2100#comment:72" title="Comment 72">jdemeyer</a>:
</p>
<blockquote class="citation">
<p>
Please rebase this patch to be applied on top of <a class="closed ticket" href="https://trac.sagemath.org/ticket/11491" title="defect: Minor doc formatting problem in tachyon_repr (closed: fixed)">#11491</a>.
</p>
</blockquote>
<p>
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.
</p>
<p>
But no point crying over spilt milk. Sigh... patches coming up.
</p>
TicketkcrismanMon, 20 Jun 2011 14:45:44 GMTattachment set
https://trac.sagemath.org/ticket/2100
https://trac.sagemath.org/ticket/2100
<ul>
<li><strong>attachment</strong>
set to <em>trac_2100-auto-automatic-and-vector.patch</em>
</li>
</ul>
<p>
Apply after rebase patch
</p>
TicketkcrismanMon, 20 Jun 2011 14:48:42 GMTstatus changed
https://trac.sagemath.org/ticket/2100#comment:74
https://trac.sagemath.org/ticket/2100#comment:74
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>positive_review</em>
</li>
</ul>
<p>
Okay, <em>now</em> we should hopefully be at positive review!
</p>
TicketjdemeyerMon, 20 Jun 2011 15:17:41 GMT
https://trac.sagemath.org/ticket/2100#comment:75
https://trac.sagemath.org/ticket/2100#comment:75
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/2100#comment:73" title="Comment 73">kcrisman</a>:
</p>
<blockquote class="citation">
<p>
Especially since Ryan and I were involved on both tickets, it would have been nice to ask first.
</p>
</blockquote>
<p>
I have done that in the past and <strong>people complained</strong>: <a class="ext-link" href="http://groups.google.com/group/sage-devel/browse_frm/thread/abd5fb944769e052/8e4057172b97f2e5"><span class="icon"></span>http://groups.google.com/group/sage-devel/browse_frm/thread/abd5fb944769e052/8e4057172b97f2e5</a>
</p>
TicketkcrismanMon, 20 Jun 2011 15:26:04 GMT
https://trac.sagemath.org/ticket/2100#comment:76
https://trac.sagemath.org/ticket/2100#comment:76
<p>
Right, and my point is that this sort of feels the same way. I am <strong>not</strong> suggesting unmerging something positively reviewed - definitely not the direction I'm going.
</p>
<p>
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 <em>before</em> deciding which one to merge first. Especially since in this case <a class="closed ticket" href="https://trac.sagemath.org/ticket/11491" title="defect: Minor doc formatting problem in tachyon_repr (closed: fixed)">#11491</a> 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.
</p>
<p>
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.
</p>
TicketjdemeyerMon, 20 Jun 2011 15:34:25 GMTwork_issues deleted
https://trac.sagemath.org/ticket/2100#comment:77
https://trac.sagemath.org/ticket/2100#comment:77
<ul>
<li><strong>work_issues</strong>
<em>rebase</em> deleted
</li>
</ul>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/2100#comment:76" title="Comment 76">kcrisman</a>:
</p>
<blockquote class="citation">
<p>
Right, and my point is that this sort of feels the same way. I am <strong>not</strong> suggesting unmerging something positively reviewed - definitely not the direction I'm going.
</p>
<p>
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 <em>before</em> deciding which one to merge first. Especially since in this case <a class="closed ticket" href="https://trac.sagemath.org/ticket/11491" title="defect: Minor doc formatting problem in tachyon_repr (closed: fixed)">#11491</a> 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.
</p>
</blockquote>
<p>
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 <a class="closed ticket" href="https://trac.sagemath.org/ticket/11491" title="defect: Minor doc formatting problem in tachyon_repr (closed: fixed)">#11491</a> before I even considered the patch here.
</p>
<p>
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.
</p>
TicketjdemeyerMon, 20 Jun 2011 15:35:27 GMTcc changed
https://trac.sagemath.org/ticket/2100#comment:78
https://trac.sagemath.org/ticket/2100#comment:78
<ul>
<li><strong>cc</strong>
<em>mpatel</em> added
</li>
</ul>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/2100#comment:76" title="Comment 76">kcrisman</a>:
</p>
<blockquote class="citation">
<p>
Thanks for all the hard work; the final releases really are noticeably more polished nowadays, and it's a great thing.
</p>
</blockquote>
<p>
You should really also thank Mitesh Patel for managing the <a class="ext-link" href="http://build.sagemath.org/sage/waterfall"><span class="icon"></span>buildbot</a>.
</p>
TicketkcrismanMon, 20 Jun 2011 15:44:33 GMT
https://trac.sagemath.org/ticket/2100#comment:79
https://trac.sagemath.org/ticket/2100#comment:79
<blockquote class="citation">
<p>
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 <a class="closed ticket" href="https://trac.sagemath.org/ticket/11491" title="defect: Minor doc formatting problem in tachyon_repr (closed: fixed)">#11491</a> before I even considered the patch here.
</p>
</blockquote>
<p>
Understood.
</p>
<blockquote class="citation">
<p>
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.
</p>
</blockquote>
<p>
Yes, that definitely makes sense, as I hope I make clear above. :)
</p>
TicketjdemeyerFri, 22 Jul 2011 12:48:14 GMTstatus changed; resolution, merged set
https://trac.sagemath.org/ticket/2100#comment:80
https://trac.sagemath.org/ticket/2100#comment:80
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>closed</em>
</li>
<li><strong>resolution</strong>
set to <em>fixed</em>
</li>
<li><strong>merged</strong>
set to <em>sage-4.7.2.alpha0</em>
</li>
</ul>
Ticket