Opened 8 years ago

Closed 7 years ago

Last modified 7 years ago

#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:


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)

trac15030.patch (2.4 KB) - added by eviatarbach 8 years ago.

Download all attachments as: .zip

Change History (14)

Changed 8 years ago by eviatarbach

comment:1 Changed 8 years ago by eviatarbach

  • Status changed from new to needs_review

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​

comment:2 Changed 8 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 8 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 7 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 7 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 7 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 7 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:8 Changed 7 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 7 years ago by rws

  • Branch set to u/rws/switch_standard_2d_plotting_from__fast_float__to__fast_callable_

comment:10 Changed 7 years ago by rws

  • Authors set to Eviatar Bach
  • 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:

7a9e07aTrac 15030: replace fast_float with fast_callable in 2D plotting

comment:11 Changed 7 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: Changed 7 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 7 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.

Note: See TracTickets for help on using tickets.