#15030 closed defect (fixed)
Switch standard 2D plotting from `fast_float` to `fast_callable`
Reported by: | eviatarbach | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-6.2 |
Component: | graphics | Keywords: | plot |
Cc: | Merged in: | ||
Authors: | Eviatar Bach | Reviewers: | Ralf Stephan |
Report Upstream: | N/A | Work issues: | compare patch with alternative by ppurka |
Branch: | 7a9e07a (Commits) | Commit: | |
Dependencies: | Stopgaps: |
Description
Switch the plot command to use fast_callable instead of fast_float. This will make commands like plot(abs(log(x)), x) work.
Attachments (1)
Change History (14)
Changed 3 years ago by eviatarbach
comment:1 Changed 3 years ago by eviatarbach
- Status changed from new to needs_review
comment:2 Changed 3 years ago by tkluck
Thanks for looking into this important issue!
Did you check to see what the performance implications of your patch are? I assume that fast_float is faster than fast_callable, but that is not based on any data.
comment:3 Changed 3 years ago by eviatarbach
Yes, fast_float is slightly faster in my tests. However, I think the plan is to deprecate fast_float anyway; see #5572.
comment:4 Changed 3 years ago by ppurka
This doesn't really fix #13559. Do you have an idea why? I thought this change to fast_callable should take care of that other ticket too.
comment:5 Changed 3 years ago by eviatarbach
I think it's because plot (look at the function generate_plot_points in sage.plot.plot) generates float points, and
sage: exp(1e5r) OverflowError: math range error
Maybe it should generate RealNumber? points instead (I guess they should also be faster than floats for many functions, because MPFR functions are functions are fast)? In any case, that doesn't affect this ticket.
comment:6 Changed 3 years ago by ppurka
Hmm.. so, you are changing only the plots for symbolic expressions. This patch makes the symbolic expression plots about five times slower.
sage: timeit('plot(abs(log(x)), (x, -20, 20))') # with patch 25 loops, best of 3: 18.5 ms per loop sage: timeit('plot(abs(log(x)), (x, -20, 20))') # without patch 125 loops, best of 3: 3.58 ms per loop
What do you think of the following change which introduces only a factor of two slowdown? The main problem seems to be that the fast float stuff does not handle complex numbers, and so we can explicitly specify the domain as CDF. I am not sure what is the default domain.
+ from sage.ext.fast_callable import fast_callable + from sage.rings.all import CDF + return fast_callable(self, vars=vars, expect_one_var=True, + domain=CDF)
The timeit results are as follows:
sage: timeit('plot(abs(log(x)), (x, -20, 20))') 125 loops, best of 3: 6 ms per loop
A few doctests in expression.pyx fail, but they can be fixed (by changing their output).
comment:7 Changed 2 years ago by vbraun_spam
- Milestone changed from sage-6.1 to sage-6.2
comment:8 Changed 2 years ago by rws
- Status changed from needs_review to needs_work
- Work issues set to compare patch with alternative by ppurka
comment:9 Changed 2 years ago by rws
- Branch set to u/rws/switch_standard_2d_plotting_from__fast_float__to__fast_callable_
comment:10 Changed 2 years ago by rws
- Commit set to 7a9e07abeaf14171706e407173a342810031e200
- Keywords plot added
- Reviewers set to Ralf Stephan
- Status changed from needs_work to positive_review
this patch before sage: timeit('plot(abs(log(x)), (x, -20, 20))') 11.4 ms 2.4 sage: timeit('circle((1,1), 1) + plot(x^2, (x,0,5))') 2.7 ms 2.3 sage: timeit('plot(x^2,(x,300,500),ticks=[None,50000])') 10.8 ms 9.4 sage: i = CDF.0 # define i this way for maximum speed. sage: timeit('plot(lambda t: arg(zeta(0.5+t*i)), 1,27,rgbcolor=(0.8,0,0))') 52.2 ms 52.2 sage: timeit('plot([sin(n*x) for n in [1..4]], (0, pi))') 10.4 ms 10.2 sage: f = (2*x^3+2*x-1)/((x-2)*(x+1)) sage: timeit('plot([f, 2*x+2], -7,7, fill = {0: [1]})') 11.2 ms 11.1
so, in my opinion, there is only a slowdown where the original behaviour was incorrect. It is not correct to say "This patch makes the symbolic expression plots about five times slower."
I rebased the patch on 6.2.beta7. LGTM and tests fine in plot/ and symbolic/.
New commits:
7a9e07a | Trac 15030: replace fast_float with fast_callable in 2D plotting |
comment:11 Changed 2 years ago by vbraun
- Branch changed from u/rws/switch_standard_2d_plotting_from__fast_float__to__fast_callable_ to 7a9e07abeaf14171706e407173a342810031e200
- Resolution set to fixed
- Status changed from positive_review to closed
comment:12 follow-up: ↓ 13 Changed 2 years ago by kcrisman
- Commit 7a9e07abeaf14171706e407173a342810031e200 deleted
Note that #13355 which is related, if not the same...
Also, anyone know how this impacts 3d plots?
comment:13 in reply to: ↑ 12 Changed 2 years ago by ppurka
Replying to kcrisman:
Also, anyone know how this impacts 3d plots?
I believe this has no impact on 3d plots. 3d plots from 2d objects goes via .plot3d() methods which usually just take the already generated data points. 3d plots created directly are not affected by this patch.
The patch changes plot to use fast_callable. It doesn't fix contour_plot or plot3d, for example, since that seems to be more involved, and should be fixed in a future patch.
Patchbot apply trac15030.patch