Opened 17 months ago

Closed 8 months ago

Last modified 7 months 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:

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)

trac15030.patch (2.4 KB) - added by eviatarbach 17 months ago.

Download all attachments as: .zip

Change History (14)

Changed 17 months ago by eviatarbach

comment:1 Changed 17 months 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 16 months 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 16 months 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 15 months 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 15 months 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 15 months 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 11 months ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:8 Changed 9 months ago by rws

  • Status changed from needs_review to needs_work
  • Work issues set to compare patch with alternative by ppurka

comment:9 Changed 8 months ago by rws

  • Branch set to u/rws/switch_standard_2d_plotting_from__fast_float__to__fast_callable_

comment:10 Changed 8 months 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 8 months 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 8 months 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 months 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.