Remove code for fast_float(old=True)

[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.

[mkoeppe]

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

[mjo]

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

[mkoeppe]

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

[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

@@ -12173 +12173 @@
         - William Stein (2007-12-07)
         """
         from sage.numerical.optimize import find_local_minimum
+        from sage.ext.fast_callable import fast_callable
 
         if var is None:
             var = self.default_variable()
-        return find_local_minimum(self._fast_float_(var),
-                                        a=a, b=b, tol=tol, maxfun=maxfun )
+
+        f = fast_callable(self, vars=[var])
+        return find_local_minimum(f, a=a, b=b, tol=tol, maxfun=maxfun)
 
     ###################
     # Fast Evaluation #

[mjo]

Numerical integration:

src/sage/calculus/integration.pyx

@@ -316 +316 @@
                 else:
                    if ell.is_numeric() and not ell.is_zero():
                       raise ValueError('integral does not converge at infinity$
-            func = func._fast_float_(v)
+            func = fast_callable(func, vars=[v])
+
 
     if isinstance(func, FastDoubleFunc):
         F.function = c_ff

[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.

[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.

[mkoeppe]

Great, thanks.

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

[git]

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

​6fd902c

src/sage/symbolic/expression_conversions.py: Remove _fast_float_, FastFloatConverter

[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?

[git]

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

​9b95060

Remove more _fast_float_ methods

[mkoeppe]

Some more things:

• src/sage/plot/plot3d/plot3d.py uses fast_float_arg

• some files import fast_callable from the wrong module

[git]

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

​6fc73e8

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

[git]

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

​b04bdbe

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

[git]

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

​927d6c2

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

[git]

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

​6b44072

src/sage/ext/fast_eval.pyx: Reduce/update documentation

[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?

[mkoeppe]

I'm referring to:

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

[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.

[mkoeppe]

I think I removed that in 927d6c2

[mkoeppe]

can you merge & push your changes please?

[mjo]

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

---

New commits:

​6bed06a	Trac #32234: use fast_callable() for symbolic find_root().
​2080a9c	Trac #32234: don't import fast_callable() from sage.ext.fast_eval.

[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

[git]

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

​f3d26be

Trac #32234: replace fast_float_arg() in plot3d.

[mjo]

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

[mkoeppe]

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

[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

[mkoeppe]

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

[git]

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

​a4545f2

Trac #32234: replace fast_float_arg() in plot3d.

[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
**********************************************************************

[git]

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

​8446bf3

Trac #32234: fix symbolic find_local_minimum() docs.

[git]

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

​1796d81

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

[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

[mkoeppe]

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

[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.

[mkoeppe]

(That's now #32268.)

[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.

[git]

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

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

[mkoeppe]

merged latest rc.

---

New commits:

​a4c840d

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

[git]

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

​72f3881

Merge tag '9.5.beta1' into t/32234/ticket/32234

[mkoeppe]

This ticket needs another reviewer...

[mjo]

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?

[mkoeppe]

That would be fine with me

[git]

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

​6fc96d6

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

[mjo]

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.

[mkoeppe]

Then let's get this in.