Opened 6 months ago

Closed 4 months ago

#32234 closed enhancement (fixed)

Remove code for fast_float(old=True)

Reported by: mkoeppe Owned by:
Priority: major Milestone: sage-9.5
Component: fast_callable Keywords:
Cc: chapoton, gh-mjungmath, egourgoulhon, vdelecroix, gh-mwageringel, kcrisman, slelievre, nbruin Merged in:
Authors: Matthias Koeppe, Michael Orlitzky Reviewers: Michael Orlitzky, Matthias Koeppe
Report Upstream: N/A Work issues:
Branch: 6fc96d6 (Commits, GitHub, GitLab) Commit: 6fc96d64d66b7e894fc0703a9324f43d54474bc4
Dependencies: Stopgaps:

Status badges

Description (last modified by mkoeppe)

The old code was replaced by new code in 2009 and is unused in the library.

$ git grep 'fast_float *(.*old *='
src/sage/ext/fast_eval.pyx:    sage: f = fast_float(sqrt(x^7+1), 'x', old=True)
src/sage/ext/fast_eval.pyx:def fast_float(f, *vars, old=None, expect_one_var=False):
src/sage/rings/polynomial/multi_polynomial.pyx:            sage: list(fast_float(K(0), old=True))
src/sage/rings/polynomial/multi_polynomial.pyx:            sage: list(fast_float(K(17), old=True))
src/sage/rings/polynomial/multi_polynomial.pyx:            sage: list(fast_float(y, old=True))

Given that this is from a time before deprecation warnings were invented, and this is clearly marked as old, we just remove this code.

We also replace the remaining direct uses of _fast_float_ methods by:

  • sage.calculus.integration.numerical_integral
  • sage.numerical.optimize.{minimize_constrained,find_fit}
  • Expression.find_local_minimum
  • Expression.find_root

Finally, we remove the entire implementation of the old fast_float (centering around class FastDoubleFunc) and all _fast_float_ methods.

Change History (59)

comment:1 Changed 6 months ago by mkoeppe

  • Description modified (diff)

comment:2 Changed 6 months ago by mkoeppe

  • Description modified (diff)

comment:3 Changed 6 months ago by mkoeppe

  • Description modified (diff)

comment:4 Changed 6 months ago by mkoeppe

  • Cc chapoton added

Cc'ing the last person who modified numerical_integral...

comment:5 Changed 6 months ago by mkoeppe

  • Branch set to u/mkoeppe/remove_code_for_fast_float_old_true_

comment:6 Changed 6 months ago by mkoeppe

  • Milestone changed from sage-9.4 to sage-9.5

comment:7 Changed 6 months ago by mjo

  • Commit set to d900e6e4a466fce9e2759ed15b076a47cee168d2

Needs review, or are you waiting to see if the three uses of _fast_float_ can be removed?

comment:8 Changed 6 months ago by mkoeppe

  • Authors set to Matthias Koeppe
  • Status changed from new to needs_review

I was kind of hoping that we can get rid of these _fast_float_-using methods, but we can of course do that later

comment:9 follow-up: Changed 6 months ago by mjo

It's unfortunate that there is a _fast_callable_ method on symbolic expressions that seems to do something else entirely... but this appears to be all that is necessary to update find_local_minimum():

  • src/sage/symbolic/expression.pyx

    diff --git a/src/sage/symbolic/expression.pyx b/src/sage/symbolic/expression.pyx
    index 0e8dc117bc..fbe74582c2 100644
    a b cdef class Expression(CommutativeRingElement): 
    1217312173        - William Stein (2007-12-07)
    1217412174        """
    1217512175        from sage.numerical.optimize import find_local_minimum
     12176        from sage.ext.fast_callable import fast_callable
    1217612177
    1217712178        if var is None:
    1217812179            var = self.default_variable()
    12179         return find_local_minimum(self._fast_float_(var),
    12180                                         a=a, b=b, tol=tol, maxfun=maxfun )
     12180
     12181        f = fast_callable(self, vars=[var])
     12182        return find_local_minimum(f, a=a, b=b, tol=tol, maxfun=maxfun)
    1218112183
    1218212184    ###################
    1218312185    # Fast Evaluation #
Last edited 6 months ago by mjo (previous) (diff)

comment:10 Changed 6 months ago by mjo

Numerical integration:

  • src/sage/calculus/integration.pyx

    diff --git a/src/sage/calculus/integration.pyx b/src/sage/calculus/integration.$
    index 93a3681854..6dd4195b2d 100644
    a b def numerical_integral(func, a, b=None, 
    316316                else:
    317317                   if ell.is_numeric() and not ell.is_zero():
    318318                      raise ValueError('integral does not converge at infinity$
    319             func = func._fast_float_(v)
     319            func = fast_callable(func, vars=[v])
     320
    320321
    321322    if isinstance(func, FastDoubleFunc):
    322323        F.function = c_ff

comment:11 in reply to: ↑ 9 Changed 6 months ago by nbruin

Replying to mjo:

It's unfortunate that there is a _fast_callable_ method on symbolic expressions that seems to do something else entirely

Possibly the name is unfortunate, but the presence of the method is actually what makes fast_callable work. Given that it implements the protocol that makes fast_callable work, I think the name is actually quite understandable.

comment:12 follow-up: Changed 6 months ago by mjo

  • Authors changed from Matthias Koeppe to Matthias Koeppe, Michael Orlitzky
  • Branch changed from u/mkoeppe/remove_code_for_fast_float_old_true_ to u/mjo/ticket/32234
  • Commit changed from d900e6e4a466fce9e2759ed15b076a47cee168d2 to 010bef3f1dece6aa3a3a5177b2523820caaf072a

I've updated the three files mentioned in the description. The doctests in those files pass but I haven't run the full suite yet.

comment:13 Changed 6 months ago by mkoeppe

Great, thanks.

If there are no problems, we can now remove more code - all _fast_float_ methods in all classes

comment:14 Changed 6 months ago by mkoeppe

  • Branch changed from u/mjo/ticket/32234 to u/mkoeppe/ticket/32234

comment:15 Changed 6 months ago by git

  • Commit changed from 010bef3f1dece6aa3a3a5177b2523820caaf072a to 6fd902cd5f76d2c114f45167712ee521865371de

Branch pushed to git repo; I updated commit sha1. New commits:

6fd902csrc/sage/symbolic/expression_conversions.py: Remove _fast_float_, FastFloatConverter

comment:16 in reply to: ↑ 12 Changed 6 months ago by mkoeppe

Replying to mjo:

I've updated the three files mentioned in the description. The doctests in those files pass but I haven't run the full suite yet.

There's another use of _fast_float_ in Expression.find_root, could you take care of this one please?

comment:17 Changed 6 months ago by git

  • Commit changed from 6fd902cd5f76d2c114f45167712ee521865371de to 9b95060539539269831024d30480b0bea2b4e114

Branch pushed to git repo; I updated commit sha1. New commits:

9b95060Remove more _fast_float_ methods

comment:18 Changed 6 months ago by mkoeppe

Some more things:

  • src/sage/plot/plot3d/plot3d.py uses fast_float_arg
  • some files import fast_callable from the wrong module

comment:19 Changed 6 months ago by git

  • Commit changed from 9b95060539539269831024d30480b0bea2b4e114 to 6fc73e8be811568f9c8511f6af6ea75078e1a7ac

Branch pushed to git repo; I updated commit sha1. New commits:

6fc73e8src/sage/ext/fast_eval.pyx: Remove everything except fast_float, is_fast_float

comment:20 Changed 6 months ago by git

  • Commit changed from 6fc73e8be811568f9c8511f6af6ea75078e1a7ac to b04bdbe49994096e68584567cca8241f0de29f04

Branch pushed to git repo; I updated commit sha1. New commits:

b04bdbesrc/sage/calculus/integration.pyx: Remove handling of FastDoubleFunc

comment:21 Changed 6 months ago by git

  • Commit changed from b04bdbe49994096e68584567cca8241f0de29f04 to 927d6c2ef5b6b0686904d4a207bc2aeca6cd2c26

Branch pushed to git repo; I updated commit sha1. New commits:

927d6c2src/sage/plot/plot3d/parametric_surface.pyx: Remove handling of FastDoubleFunc

comment:22 Changed 6 months ago by git

  • Commit changed from 927d6c2ef5b6b0686904d4a207bc2aeca6cd2c26 to 6b44072ee77ca7bc5fe276b9d54bcf37662c0218

Branch pushed to git repo; I updated commit sha1. New commits:

6b44072src/sage/ext/fast_eval.pyx: Reduce/update documentation

comment:23 Changed 6 months ago by mjo

Replying to mkoeppe:

  • some files import fast_callable from the wrong module

There's a different fast_callable function in sage/symbolic/expression_conversions.py, if that's what you're seeing?

comment:24 Changed 6 months ago by mkoeppe

I'm referring to:

src/sage/numerical/optimize.py:    from sage.ext.fast_eval import fast_callable

comment:25 Changed 6 months ago by mjo

Ok, I fixed that and the same import in sage/calculus/riemann.pyx. I think I've fixed find_root() too but sage -b is complaining about FastDoubleFunc in sage/plot/plot3d/parametric_surface.pyx at the moment.

comment:26 Changed 6 months ago by mkoeppe

I think I removed that in 927d6c2

comment:27 Changed 6 months ago by mkoeppe

can you merge & push your changes please?

comment:28 Changed 6 months ago by mjo

  • Branch changed from u/mkoeppe/ticket/32234 to u/mjo/ticket/32234
  • Commit changed from 6b44072ee77ca7bc5fe276b9d54bcf37662c0218 to 2080a9c3bfd3af58c5295075853c3a69c99e2382

Sure, I was trying to fix the fast_float_arg thing, but here are the two aforementioned commits.


New commits:

6bed06aTrac #32234: use fast_callable() for symbolic find_root().
2080a9cTrac #32234: don't import fast_callable() from sage.ext.fast_eval.

comment:29 Changed 6 months ago by mjo

FWIW here's a way to mimic fast_float_arg(0) and fast_float_arg(1):

sage: u = SR.symbol('u', domain='real')                                         
sage: v = SR.symbol('v', domain='real')                                         
sage: u = fast_callable(u, vars=[u,v])                                          
sage: v = fast_callable(v, vars=[u,v])
sage: u(1,2)                                                                    
1
sage: v(1,2)                                                                    
2

comment:30 Changed 6 months ago by mkoeppe

  • Description modified (diff)

comment:31 Changed 6 months ago by git

  • Commit changed from 2080a9c3bfd3af58c5295075853c3a69c99e2382 to f3d26be964e76deccbdb51758e0739a4506ac345

Branch pushed to git repo; I updated commit sha1. New commits:

f3d26beTrac #32234: replace fast_float_arg() in plot3d.

comment:32 Changed 6 months ago by mjo

Using SR feels ugly there, but for lack of a better idea, it works.

comment:33 Changed 6 months ago by mkoeppe

I would be concerned that this puts global assumptions on these two variables

comment:34 Changed 6 months ago by mjo

I could either remove the domain='real', or replace the whole thing with e.g.

sage: R = PolynomialRing(RDF, 'u,v')                                            
sage: u = fast_callable(R.gen(0), vars=R.gens())                                
sage: v = fast_callable(R.gen(1), vars=R.gens())                                
sage: u(1,2)                                                                    
1.0
sage: v(1,2)                                                                    
2.0

comment:35 follow-up: Changed 6 months ago by mkoeppe

either way should be fine, or perhaps use ExpressionTreeBuilder.var directly

comment:36 Changed 6 months ago by git

  • Commit changed from f3d26be964e76deccbdb51758e0739a4506ac345 to a4545f2fc51cd256c16d437a5bc49ae5323ae9e6

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

a4545f2Trac #32234: replace fast_float_arg() in plot3d.

comment:37 Changed 6 months ago by mkoeppe

Looking great, but I'm getting a few doctest errors

sage -t --random-seed=0 src/sage/tests/books/computational-mathematics-with-sagemath/calculus_doctest.py
**********************************************************************
File "src/sage/tests/books/computational-mathematics-with-sagemath/calculus_doctest.py", line 353, in sage.tests.books.computational-mathematics-with-sagemath.calculus_doctest
Failed example:
    n0 = find_root(u(n) - 1e-8 == 0, 22, 1000); n0
Exception raised:
    Traceback (most recent call last):
      File "/Users/mkoeppe/s/sage/sage-rebasing/worktree-rebase/.tox/local-homebrew-macos-minimal/local/lib/python3.9/site-packages/sage/doctest/forker.py", line 718, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/Users/mkoeppe/s/sage/sage-rebasing/worktree-rebase/.tox/local-homebrew-macos-minimal/local/lib/python3.9/site-packages/sage/doctest/forker.py", line 1137, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.tests.books.computational-mathematics-with-sagemath.calculus_doctest[89]>", line 1, in <module>
        n0 = find_root(u(n) - RealNumber('1e-8') == Integer(0), Integer(22), Integer(1000)); n0
      File "sage/misc/lazy_import.pyx", line 362, in sage.misc.lazy_import.LazyImport.__call__ (build/cythonized/sage/misc/lazy_import.c:4036)
        return self.get_object()(*args, **kwds)
      File "/Users/mkoeppe/s/sage/sage-rebasing/worktree-rebase/.tox/local-homebrew-macos-minimal/local/lib/python3.9/site-packages/sage/numerical/optimize.py", line 105, in find_root
        return f.find_root(a=a,b=b,xtol=xtol,rtol=rtol,maxiter=maxiter,full_output=full_output)
      File "sage/symbolic/expression.pyx", line 12098, in sage.symbolic.expression.Expression.find_root (build/cythonized/sage/symbolic/expression.cpp:64387)
        return find_root(f, a=a, b=b, xtol=xtol,
      File "/Users/mkoeppe/s/sage/sage-rebasing/worktree-rebase/.tox/local-homebrew-macos-minimal/local/lib/python3.9/site-packages/sage/numerical/optimize.py", line 113, in find_root
        right = f(b)
      File "sage/ext/interpreters/wrapper_py.pyx", line 68, in sage.ext.interpreters.wrapper_py.Wrapper_py.__call__ (build/cythonized/sage/ext/interpreters/wrapper_py.c:2040)
        raise
      File "sage/ext/interpreters/wrapper_py.pyx", line 60, in sage.ext.interpreters.wrapper_py.Wrapper_py.__call__ (build/cythonized/sage/ext/interpreters/wrapper_py.c:1974)
        return interp_py((<PyTupleObject*>args).ob_item
      File "sage/rings/integer.pyx", line 2206, in sage.rings.integer.Integer.__pow__ (build/cythonized/sage/rings/integer.c:15193)
        return coercion_model.bin_op(left, right, operator.pow)
      File "sage/structure/coerce.pyx", line 1204, in sage.structure.coerce.CoercionModel.bin_op (build/cythonized/sage/structure/coerce.c:10671)
        return PyObject_CallObject(op, xy)
    OverflowError: (34, 'Result too large')
**********************************************************************
sage -t --random-seed=0 src/sage/numerical/optimize.py
**********************************************************************
File "src/sage/numerical/optimize.py", line 81, in sage.numerical.optimize.find_root
Failed example:
    find_root(x^2*log(x,2)-1,0, 2)  # abs tol 1e-6
Expected:
    1.41421356237
Got:
    0.0
Tolerance exceeded:
    1.41421356237 vs 0.0, tolerance 2e0 > 1e-6
**********************************************************************
File "src/sage/numerical/optimize.py", line 98, in sage.numerical.optimize.find_root
Failed example:
    find_root(f, -1, 0)
Expected:
    Traceback (most recent call last):
    ...
    RuntimeError: f appears to have no zero on the interval
Got:
    0.0
**********************************************************************

comment:38 Changed 6 months ago by git

  • Commit changed from a4545f2fc51cd256c16d437a5bc49ae5323ae9e6 to 8446bf39a1eac8e44b57b98e250ccbeb93217c14

Branch pushed to git repo; I updated commit sha1. New commits:

8446bf3Trac #32234: fix symbolic find_local_minimum() docs.

comment:39 Changed 6 months ago by git

  • Commit changed from 8446bf39a1eac8e44b57b98e250ccbeb93217c14 to 1796d81e54cbe3bf756927aeaca599511bff5f75

Branch pushed to git repo; I updated commit sha1. New commits:

1796d81Trac #32234: use domain=float for fast callables used by numpy.

comment:40 Changed 6 months ago by mjo

Should be fixed now. Numpy didn't know what to do about NaN unless domain=float was specified.

Not important, but one of those example functions isn't doing what the doctest thinks it does:

sage: f(x) = 0.0 / max(0, x)                                                                                                                                                 
sage: f                                                                                                                                                                      
x |--> NaN

since

sage: max(0, x)                                                                                                                                                              
0

comment:41 Changed 6 months ago by mkoeppe

  • Cc gh-mjungmath egourgoulhon added
  • Reviewers set to Matthias Koeppe, ...

As this ticket makes changes to plotting functions, it would be good to check that there are no speed regressions

comment:42 Changed 6 months ago by mkoeppe

In the next step, we could deprecate the compatibility function fast_float and switch all uses to direct calls to fast_callable. Perhaps best in a follow-up ticket.

comment:43 Changed 6 months ago by mkoeppe

(That's now #32268.)

comment:44 Changed 6 months ago by mkoeppe

  • Cc vdelecroix added

comment:45 in reply to: ↑ 35 Changed 6 months ago by mjo

Replying to mkoeppe:

either way should be fine, or perhaps use ExpressionTreeBuilder.var directly

Feeling a bit stupid right now because lambda u,v: v is about four times as fast as the complicated fast-callable-arg2 stuff on my machine.

comment:46 Changed 6 months ago by git

  • Commit changed from 1796d81e54cbe3bf756927aeaca599511bff5f75 to df062b30005bf8488fa16124cd7c2b9129e7b396

Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:

1377741src/sage/ext/fast_eval.pyx: Remove everything except fast_float, is_fast_float
0807522src/sage/calculus/integration.pyx: Remove handling of FastDoubleFunc
9199406src/sage/plot/plot3d/parametric_surface.pyx: Remove handling of FastDoubleFunc
e6f88d6src/sage/ext/fast_eval.pyx: Reduce/update documentation
1353bafTrac #32234: use fast_callable() for symbolic find_root().
c56fa05Trac #32234: don't import fast_callable() from sage.ext.fast_eval.
0fcec8eTrac #32234: replace fast_float_arg() in plot3d.
1479666Trac #32234: fix symbolic find_local_minimum() docs.
19aa975Trac #32234: use domain=float for fast callables used by numpy.
df062b3Trac #32234: replace two trivial fast-callables in plot3d().

comment:47 Changed 6 months ago by mkoeppe

  • Cc gh-mwageringel added

comment:48 Changed 5 months ago by mkoeppe

  • Cc kcrisman slelievre added

comment:49 Changed 5 months ago by mkoeppe

  • Branch changed from u/mjo/ticket/32234 to u/mkoeppe/ticket/32234

comment:50 Changed 5 months ago by mkoeppe

  • Commit changed from df062b30005bf8488fa16124cd7c2b9129e7b396 to a4c840df6311ff89434e559fa079f399b354fa18

merged latest rc.


New commits:

a4c840dMerge tag '9.4.rc2' into t/32234/ticket/32234

comment:51 Changed 4 months ago by git

  • Commit changed from a4c840df6311ff89434e559fa079f399b354fa18 to 72f388109b770a03b370a8730aa666ab95fe0e78

Branch pushed to git repo; I updated commit sha1. New commits:

72f3881Merge tag '9.5.beta1' into t/32234/ticket/32234

comment:52 Changed 4 months ago by mkoeppe

This ticket needs another reviewer...

comment:53 Changed 4 months ago by mkoeppe

  • Cc nbruin added

comment:54 Changed 4 months ago by mjo

  • Branch changed from u/mkoeppe/ticket/32234 to u/mjo/ticket/32234
  • Commit changed from 72f388109b770a03b370a8730aa666ab95fe0e78 to 96ba1213fa281beba55d95f15352b09699eea45f

I looked through this one more time, and noticed that domain=float can be used in the earlier commits as well (the functions/results get passed to external library routines expecting floats anyway). I've added a commit to do that, and rebased onto beta2.

Shall we say we've eached reviewed the other's contribution here?

comment:55 Changed 4 months ago by mkoeppe

That would be fine with me

comment:56 Changed 4 months ago by git

  • Commit changed from 96ba1213fa281beba55d95f15352b09699eea45f to 6fc96d64d66b7e894fc0703a9324f43d54474bc4

Branch pushed to git repo; I updated commit sha1. New commits:

6fc96d6Trac #32234: fix pyflakes warning (missing scipy import).

comment:57 Changed 4 months ago by mjo

  • Reviewers changed from Matthias Koeppe, ... to Matthias Koeppe, Michael Orlitzky

Ok, I added one more commit to fix a missing(?) import, or to at least satisfy the patchbots. I'm happy with your commits if you're happy with mine.

comment:58 Changed 4 months ago by mkoeppe

  • Reviewers changed from Matthias Koeppe, Michael Orlitzky to Michael Orlitzky, Matthias Koeppe
  • Status changed from needs_review to positive_review

Then let's get this in.

comment:59 Changed 4 months ago by vbraun

  • Branch changed from u/mjo/ticket/32234 to 6fc96d64d66b7e894fc0703a9324f43d54474bc4
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.