Sage: Ticket #7008: [with patch, positive review] consolidate in plotting all extraction of variables, ranges, and fast_float setup
https://trac.sagemath.org/ticket/7008
<p>
Currently, code for extracting variables, dealing with ranges of variables, and making the functions fast_float is scattered throughout the plotting directory. There are multiple implementations, each having its own quirks.
</p>
<p>
This patch consolidates all of this to two functions in sage.plot.misc and makes all the necessary changes to use this consolidated function.
</p>
en-usSagehttps://trac.sagemath.org/chrome/site/logo_sagemath_trac.png
https://trac.sagemath.org/ticket/7008
Trac 1.1.6jasonFri, 25 Sep 2009 04:14:51 GMTattachment set
https://trac.sagemath.org/ticket/7008
https://trac.sagemath.org/ticket/7008
<ul>
<li><strong>attachment</strong>
set to <em>trac-7008-refactor-setup-eval-on-grid.patch</em>
</li>
</ul>
TicketjasonFri, 25 Sep 2009 04:15:57 GMTcc changed
https://trac.sagemath.org/ticket/7008#comment:1
https://trac.sagemath.org/ticket/7008#comment:1
<ul>
<li><strong>cc</strong>
<em>rbeezer</em> added
</li>
</ul>
TicketjasonFri, 25 Sep 2009 04:17:24 GMTcc changed
https://trac.sagemath.org/ticket/7008#comment:2
https://trac.sagemath.org/ticket/7008#comment:2
<ul>
<li><strong>cc</strong>
<em>robertwb</em> added
</li>
</ul>
TicketjasonFri, 25 Sep 2009 05:43:41 GMTsummary changed
https://trac.sagemath.org/ticket/7008#comment:3
https://trac.sagemath.org/ticket/7008#comment:3
<ul>
<li><strong>summary</strong>
changed from <em>consolidate in plotting all extraction of variables, ranges, and fast_float setup</em> to <em>[with patch, needs review] consolidate in plotting all extraction of variables, ranges, and fast_float setup</em>
</li>
</ul>
TicketjasonFri, 25 Sep 2009 05:59:29 GMT
https://trac.sagemath.org/ticket/7008#comment:4
https://trac.sagemath.org/ticket/7008#comment:4
<p>
I believe this depends on <a class="closed ticket" href="https://trac.sagemath.org/ticket/6985" title="defect: [with patch, positive review] complex_plot needs to use fast_callable (closed: fixed)">#6985</a>
</p>
TicketkcrismanFri, 25 Sep 2009 14:58:21 GMTsummary changed; author set
https://trac.sagemath.org/ticket/7008#comment:5
https://trac.sagemath.org/ticket/7008#comment:5
<ul>
<li><strong>summary</strong>
changed from <em>[with patch, needs review] consolidate in plotting all extraction of variables, ranges, and fast_float setup</em> to <em>[with patch, needs work] consolidate in plotting all extraction of variables, ranges, and fast_float setup</em>
</li>
<li><strong>author</strong>
set to <em>Jason Grout</em>
</li>
</ul>
<p>
Cursory review reveals a bit of a slowdown. Try even
</p>
<pre class="wiki">sage: f(x)=sin(x)
sage: timeit('plot(f,(-pi,pi))')
</pre><p>
Also notice that
</p>
<pre class="wiki">sage: timeit('plot(f(x),(-pi,pi))')
</pre><p>
is even slower yet. These are all milliseconds, not seconds, of course, but in a large plot of many functions, a contour plot with a very dense grid, or for making interacts snappy... Is there any way to fix that?
</p>
<p>
Also, what is the rationale for using fast_float and not fast_callable? I ask only because there has been so much talk about "we should really use fast_callable instead of fast_float for X". But I may not understand the difference between the two.
</p>
<p>
Finally, there should be at least one test to demonstrate that
</p>
<pre class="wiki">sage: plot(f, -pi, pi)
</pre><p>
and friends still work in the one-variable case, because there is no reason for that to disappear in the name of consistency. The more parentheses, the more confused students get. But it can be a test, since of course we don't have to promote that syntax.
</p>
<p>
These are all nitpicky. Nice refactoring, and all tests pass!
</p>
TicketjasonFri, 25 Sep 2009 21:18:46 GMT
https://trac.sagemath.org/ticket/7008#comment:6
https://trac.sagemath.org/ticket/7008#comment:6
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/7008#comment:5" title="Comment 5">kcrisman</a>:
</p>
<blockquote class="citation">
<p>
Cursory review reveals a bit of a slowdown. Try even
</p>
<pre class="wiki">sage: f(x)=sin(x)
sage: timeit('plot(f,(-pi,pi))')
</pre><p>
Also notice that
</p>
<pre class="wiki">sage: timeit('plot(f(x),(-pi,pi))')
</pre><p>
is even slower yet. These are all milliseconds, not seconds, of course, but in a large plot of many functions, a contour plot with a very dense grid, or for making interacts snappy... Is there any way to fix that?
</p>
</blockquote>
<p>
I will check into this. My guess is that any slowdown is in a fixed cost, not a marginal cost, so the slowdown should not scale. Can you test, for example, contour plots with varying grid sizes (before/after the patch) to see if there is a huge slowdown?
</p>
<p>
I wonder how much moving this function to cython might help. Well, I guess checking and then profiling is the right course of action.
</p>
<blockquote class="citation">
<p>
Also, what is the rationale for using fast_float and not fast_callable? I ask only because there has been so much talk about "we should really use fast_callable instead of fast_float for X". But I may not understand the difference between the two.
</p>
</blockquote>
<p>
fast_callable has not matured to the point that we need it yet. For example, fast_callable(0) does not work, whereas fast_float(0) does. <a class="needs_work ticket" href="https://trac.sagemath.org/ticket/5572" title="defect: fast_callable improvements (followup for #5093) (needs_work)">#5572</a> lists some of the things that fast_callable needs to still implement.
</p>
<p>
That said, fast_float uses fast_callable where it can. So for most functions, I am actually using fast_callable underneath things.
</p>
<blockquote class="citation">
<p>
Finally, there should be at least one test to demonstrate that
</p>
<pre class="wiki">sage: plot(f, -pi, pi)
</pre><p>
and friends still work in the one-variable case, because there is no reason for that to disappear in the name of consistency. The more parentheses, the more confused students get. But it can be a test, since of course we don't have to promote that syntax.
</p>
</blockquote>
<p>
Hmmm...did I delete a test doing that? I didn't mean to.
</p>
<p>
I did change the parametric_plot syntax to deprecate ranges without parenthesis. This was to maintain consistency with the 3d parametric plot. For example, currently, parametric_plot((t,t<sup>2), 0, 2*pi) works, but parametric_plot((t,t</sup>2,t<sup>3), 0, 2*pi) does not work.
</sup></p>
<blockquote class="citation">
<p>
These are all nitpicky. Nice refactoring, and all tests pass!
</p>
</blockquote>
<p>
Thanks!
</p>
TicketkcrismanSat, 26 Sep 2009 02:38:22 GMTsummary changed; reviewer set
https://trac.sagemath.org/ticket/7008#comment:7
https://trac.sagemath.org/ticket/7008#comment:7
<ul>
<li><strong>reviewer</strong>
set to <em>Karl-Dieter Crisman</em>
</li>
<li><strong>summary</strong>
changed from <em>[with patch, needs work] consolidate in plotting all extraction of variables, ranges, and fast_float setup</em> to <em>[with patch, positive review] consolidate in plotting all extraction of variables, ranges, and fast_float setup</em>
</li>
</ul>
<blockquote class="citation">
<p>
I will check into this. My guess is that any slowdown is in a fixed cost, not a marginal cost, so the slowdown should not scale. Can you test, for example, contour plots with varying grid sizes (before/after the patch) to see if there is a huge slowdown?
</p>
</blockquote>
<p>
I had already done some of this. After trying some more stuff, it looks like it's just not enough to be worthwhile to do anything else - possibly a little more than fixed, but not much. Just thought you might have a thought.
</p>
<blockquote class="citation">
<p>
That said, fast_float uses fast_callable where it can. So for most functions, I am actually using fast_callable underneath things.
</p>
</blockquote>
<p>
Ah, I didn't know that. Neat.
</p>
<blockquote class="citation">
<blockquote class="citation">
<p>
Finally, there should be at least one test to demonstrate that
</p>
<pre class="wiki">sage: plot(f, -pi, pi)
</pre><p>
and friends still work in the one-variable case, because there is no reason for that to disappear in the name of consistency. The more parentheses, the more confused students get. But it can be a test, since of course we don't have to promote that syntax.
</p>
</blockquote>
<p>
Hmmm...did I delete a test doing that? I didn't mean to.
</p>
</blockquote>
<p>
Didn't delete, but a couple of ones in graphics_array got changed. Not a big deal, as there are plenty of others in the main docs - I just didn't have time to check if those were the only ones earlier today.
</p>
<p>
I like the use of "ignore".
</p>
<p>
Okay, code checks out, and a bevy of examples don't just pass tests but also look like they are supposed to :)
</p>
TicketmvnguSat, 26 Sep 2009 03:30:42 GMTsummary changed
https://trac.sagemath.org/ticket/7008#comment:8
https://trac.sagemath.org/ticket/7008#comment:8
<ul>
<li><strong>summary</strong>
changed from <em>[with patch, positive review] consolidate in plotting all extraction of variables, ranges, and fast_float setup</em> to <em>[with patch, needs work] consolidate in plotting all extraction of variables, ranges, and fast_float setup</em>
</li>
</ul>
<p>
I'm getting the following doctest failures, all of which are deprecation warnings:
</p>
<pre class="wiki">sage -t -long devel/sage/doc/en/tutorial/tour_plotting.rst
**********************************************************************
File "/scratch/mvngu/release/sage-4.1.2.alpha2/devel/sage-main/doc/en/tutorial/tour_plotting.rst", line 176:
sage: implicit_plot3d(x^2 + y^2 + z^2 - 4, (-2, 2), (-2, 2), (-2, 2))
Expected nothing
Got:
doctest:245: DeprecationWarning: Unnamed ranges for more than one variable is deprecated and will be removed from a future release of Sage; you can used named ranges instead, like (x,0,2)
<BLANKLINE>
**********************************************************************
File "/scratch/mvngu/release/sage-4.1.2.alpha2/devel/sage-main/doc/en/tutorial/tour_plotting.rst", line 74:
sage: parametric_plot((cos(x),sin(x)^3),0,2*pi,rgbcolor=hue(0.6))
Expected nothing
Got:
doctest:1: DeprecationWarning: variable ranges to parametric_plot must be given as tuples, like (2,4) or (t,2,3)
<BLANKLINE>
**********************************************************************
File "/scratch/mvngu/release/sage-4.1.2.alpha2/devel/sage-main/doc/en/tutorial/tour_plotting.rst", line 81:
sage: p1 = parametric_plot((cos(x),sin(x)),0,2*pi,rgbcolor=hue(0.2))
Expected nothing
Got:
doctest:1: DeprecationWarning: variable ranges to parametric_plot must be given as tuples, like (2,4) or (t,2,3)
**********************************************************************
3 items had failures:
1 of 4 in __main__.example_13
1 of 4 in __main__.example_4
1 of 7 in __main__.example_5
***Test Failed*** 3 failures.
For whitespace errors, see the file /home/mvngu/.sage//tmp/.doctest_tour_plotting.py
[10.1 s]
<SNIP>
sage -t -long devel/sage/doc/fr/tutorial/tour_plotting.rst
**********************************************************************
File "/scratch/mvngu/release/sage-4.1.2.alpha2/devel/sage-main/doc/fr/tutorial/tour_plotting.rst", line 75:
sage: parametric_plot((cos(x),sin(x)^3),0,2*pi,rgbcolor=hue(0.6))
Expected nothing
Got:
doctest:1: DeprecationWarning: variable ranges to parametric_plot must be given as tuples, like (2,4) or (t,2,3)
<BLANKLINE>
**********************************************************************
File "/scratch/mvngu/release/sage-4.1.2.alpha2/devel/sage-main/doc/fr/tutorial/tour_plotting.rst", line 82:
sage: p1 = parametric_plot((cos(x),sin(x)),0,2*pi,rgbcolor=hue(0.2))
Expected nothing
Got:
doctest:1: DeprecationWarning: variable ranges to parametric_plot must be given as tuples, like (2,4) or (t,2,3)
**********************************************************************
2 items had failures:
1 of 4 in __main__.example_4
1 of 7 in __main__.example_5
***Test Failed*** 2 failures.
For whitespace errors, see the file /home/mvngu/.sage//tmp/.doctest_tour_plotting.py
[8.0 s]
<SNIP>
sage -t -long devel/sage/sage/calculus/desolvers.py
**********************************************************************
File "/scratch/mvngu/release/sage-4.1.2.alpha2/devel/sage-main/sage/calculus/desolvers.py", line 235:
sage: parametric_plot((solnx,solny),0,1)
Expected nothing
Got:
doctest:1: DeprecationWarning: variable ranges to parametric_plot must be given as tuples, like (2,4) or (t,2,3)
<BLANKLINE>
**********************************************************************
File "/scratch/mvngu/release/sage-4.1.2.alpha2/devel/sage-main/sage/calculus/desolvers.py", line 307:
sage: P2 = parametric_plot((solnx,solny),0,1)
Expected nothing
Got:
doctest:1: DeprecationWarning: variable ranges to parametric_plot must be given as tuples, like (2,4) or (t,2,3)
**********************************************************************
2 items had failures:
1 of 12 in __main__.example_4
1 of 17 in __main__.example_5
***Test Failed*** 2 failures.
For whitespace errors, see the file /home/mvngu/.sage//tmp/.doctest_desolvers.py
[6.2 s]
<SNIP>
sage -t -long devel/sage/doc/en/tutorial/tour_algebra.rst
**********************************************************************
File "/scratch/mvngu/release/sage-4.1.2.alpha2/devel/sage-main/doc/en/tutorial/tour_algebra.rst", line 263:
sage: P = parametric_plot((cos(2*t) + 2*cos(t), 4*cos(t) - cos(2*t) ), 0, 2*pi, rgbcolor=hue(0.9))
Expected nothing
Got:
doctest:1: DeprecationWarning: variable ranges to parametric_plot must be given as tuples, like (2,4) or (t,2,3)
**********************************************************************
1 items had failures:
1 of 5 in __main__.example_15
***Test Failed*** 1 failures.
For whitespace errors, see the file /home/mvngu/.sage//tmp/.doctest_tour_algebra.py
[5.8 s]
<SNIP>
sage -t -long devel/sage/doc/fr/tutorial/tour_algebra.rst
**********************************************************************
File "/scratch/mvngu/release/sage-4.1.2.alpha2/devel/sage-main/doc/fr/tutorial/tour_algebra.rst", line 242:
sage: P = parametric_plot((cos(2*t) + 2*cos(t), 4*cos(t) - cos(2*t) ), 0, 2*pi, rgbcolor=hue(0.9))
Expected nothing
Got:
doctest:1: DeprecationWarning: variable ranges to parametric_plot must be given as tuples, like (2,4) or (t,2,3)
**********************************************************************
1 items had failures:
1 of 5 in __main__.example_15
***Test Failed*** 1 failures.
For whitespace errors, see the file /home/mvngu/.sage//tmp/.doctest_tour_algebra.py
[5.7 s]
</pre>
TicketjasonSat, 26 Sep 2009 04:05:57 GMT
https://trac.sagemath.org/ticket/7008#comment:9
https://trac.sagemath.org/ticket/7008#comment:9
<p>
The implicit_plot3d warnings should have been turned on before, but they weren't since they used their own version of the setup function, and so we missed them when we did the deprecation warnings. So I'll just fix those tests.
</p>
<p>
The parametric_plot deprecation warnings are new with this patch, though (as pointed about above). However, especially if they were in our basic documentation like the tour, maybe I should bring this up on sage-devel.
</p>
TicketjasonTue, 29 Sep 2009 06:06:26 GMTattachment set
https://trac.sagemath.org/ticket/7008
https://trac.sagemath.org/ticket/7008
<ul>
<li><strong>attachment</strong>
set to <em>trac-7008-doctests.patch</em>
</li>
</ul>
<p>
apply on top of previous patch
</p>
TicketjasonTue, 29 Sep 2009 06:08:12 GMTsummary changed
https://trac.sagemath.org/ticket/7008#comment:10
https://trac.sagemath.org/ticket/7008#comment:10
<ul>
<li><strong>summary</strong>
changed from <em>[with patch, needs work] consolidate in plotting all extraction of variables, ranges, and fast_float setup</em> to <em>[with patch, needs review] consolidate in plotting all extraction of variables, ranges, and fast_float setup</em>
</li>
</ul>
<p>
I posted about the parametric_plot deprecation and no one opposed the deprecation. So the new patch should fix everything up just right.
</p>
TicketkcrismanTue, 29 Sep 2009 14:35:18 GMTsummary changed
https://trac.sagemath.org/ticket/7008#comment:11
https://trac.sagemath.org/ticket/7008#comment:11
<ul>
<li><strong>summary</strong>
changed from <em>[with patch, needs review] consolidate in plotting all extraction of variables, ranges, and fast_float setup</em> to <em>[with patch, positive review] consolidate in plotting all extraction of variables, ranges, and fast_float setup</em>
</li>
</ul>
<p>
All looks good, doctests pass.
</p>
TicketmvnguTue, 29 Sep 2009 14:53:29 GMTstatus changed; resolution, merged set
https://trac.sagemath.org/ticket/7008#comment:12
https://trac.sagemath.org/ticket/7008#comment:12
<ul>
<li><strong>status</strong>
changed from <em>new</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.1.2.rc0</em>
</li>
</ul>
<p>
Merged both patches.
</p>
Ticket