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:  sage9.5 
Component:  fast_callable  Keywords:  
Cc:  chapoton, ghmjungmath, egourgoulhon, vdelecroix, ghmwageringel, 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: 
Description (last modified by )
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
 Description modified (diff)
comment:2 Changed 6 months ago by
 Description modified (diff)
comment:3 Changed 6 months ago by
 Description modified (diff)
comment:4 Changed 6 months ago by
 Cc chapoton added
comment:5 Changed 6 months ago by
 Branch set to u/mkoeppe/remove_code_for_fast_float_old_true_
comment:6 Changed 6 months ago by
 Milestone changed from sage9.4 to sage9.5
comment:7 Changed 6 months ago by
 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
 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 followup: ↓ 11 Changed 6 months ago by
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): 12173 12173  William Stein (20071207) 12174 12174 """ 12175 12175 from sage.numerical.optimize import find_local_minimum 12176 from sage.ext.fast_callable import fast_callable 12176 12177 12177 12178 if var is None: 12178 12179 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) 12181 12183 12182 12184 ################### 12183 12185 # Fast Evaluation #
comment:10 Changed 6 months ago by
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, 316 316 else: 317 317 if ell.is_numeric() and not ell.is_zero(): 318 318 raise ValueError('integral does not converge at infinity$ 319 func = func._fast_float_(v) 319 func = fast_callable(func, vars=[v]) 320 320 321 321 322 if isinstance(func, FastDoubleFunc): 322 323 F.function = c_ff
comment:11 in reply to: ↑ 9 Changed 6 months ago by
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 followup: ↓ 16 Changed 6 months ago by
 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
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
 Branch changed from u/mjo/ticket/32234 to u/mkoeppe/ticket/32234
comment:15 Changed 6 months ago by
 Commit changed from 010bef3f1dece6aa3a3a5177b2523820caaf072a to 6fd902cd5f76d2c114f45167712ee521865371de
Branch pushed to git repo; I updated commit sha1. New commits:
6fd902c  src/sage/symbolic/expression_conversions.py: Remove _fast_float_, FastFloatConverter

comment:16 in reply to: ↑ 12 Changed 6 months ago by
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
 Commit changed from 6fd902cd5f76d2c114f45167712ee521865371de to 9b95060539539269831024d30480b0bea2b4e114
Branch pushed to git repo; I updated commit sha1. New commits:
9b95060  Remove more _fast_float_ methods

comment:18 Changed 6 months ago by
Some more things:
src/sage/plot/plot3d/plot3d.py
usesfast_float_arg
 some files import
fast_callable
from the wrong module
comment:19 Changed 6 months ago by
 Commit changed from 9b95060539539269831024d30480b0bea2b4e114 to 6fc73e8be811568f9c8511f6af6ea75078e1a7ac
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

comment:20 Changed 6 months ago by
 Commit changed from 6fc73e8be811568f9c8511f6af6ea75078e1a7ac to b04bdbe49994096e68584567cca8241f0de29f04
Branch pushed to git repo; I updated commit sha1. New commits:
b04bdbe  src/sage/calculus/integration.pyx: Remove handling of FastDoubleFunc

comment:21 Changed 6 months ago by
 Commit changed from b04bdbe49994096e68584567cca8241f0de29f04 to 927d6c2ef5b6b0686904d4a207bc2aeca6cd2c26
Branch pushed to git repo; I updated commit sha1. New commits:
927d6c2  src/sage/plot/plot3d/parametric_surface.pyx: Remove handling of FastDoubleFunc

comment:22 Changed 6 months ago by
 Commit changed from 927d6c2ef5b6b0686904d4a207bc2aeca6cd2c26 to 6b44072ee77ca7bc5fe276b9d54bcf37662c0218
Branch pushed to git repo; I updated commit sha1. New commits:
6b44072  src/sage/ext/fast_eval.pyx: Reduce/update documentation

comment:23 Changed 6 months ago by
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
I'm referring to:
src/sage/numerical/optimize.py: from sage.ext.fast_eval import fast_callable
comment:25 Changed 6 months ago by
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
I think I removed that in 927d6c2
comment:27 Changed 6 months ago by
can you merge & push your changes please?
comment:28 Changed 6 months ago by
 Branch changed from u/mkoeppe/ticket/32234 to u/mjo/ticket/32234
 Commit changed from 6b44072ee77ca7bc5fe276b9d54bcf37662c0218 to 2080a9c3bfd3af58c5295075853c3a69c99e2382
comment:29 Changed 6 months ago by
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
 Description modified (diff)
comment:31 Changed 6 months ago by
 Commit changed from 2080a9c3bfd3af58c5295075853c3a69c99e2382 to f3d26be964e76deccbdb51758e0739a4506ac345
Branch pushed to git repo; I updated commit sha1. New commits:
f3d26be  Trac #32234: replace fast_float_arg() in plot3d.

comment:32 Changed 6 months ago by
Using SR
feels ugly there, but for lack of a better idea, it works.
comment:33 Changed 6 months ago by
I would be concerned that this puts global assumptions on these two variables
comment:34 Changed 6 months ago by
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 followup: ↓ 45 Changed 6 months ago by
either way should be fine, or perhaps use ExpressionTreeBuilder.var
directly
comment:36 Changed 6 months ago by
 Commit changed from f3d26be964e76deccbdb51758e0739a4506ac345 to a4545f2fc51cd256c16d437a5bc49ae5323ae9e6
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.

comment:37 Changed 6 months ago by
Looking great, but I'm getting a few doctest errors
sage t randomseed=0 src/sage/tests/books/computationalmathematicswithsagemath/calculus_doctest.py ********************************************************************** File "src/sage/tests/books/computationalmathematicswithsagemath/calculus_doctest.py", line 353, in sage.tests.books.computationalmathematicswithsagemath.calculus_doctest Failed example: n0 = find_root(u(n)  1e8 == 0, 22, 1000); n0 Exception raised: Traceback (most recent call last): File "/Users/mkoeppe/s/sage/sagerebasing/worktreerebase/.tox/localhomebrewmacosminimal/local/lib/python3.9/sitepackages/sage/doctest/forker.py", line 718, in _run self.compile_and_execute(example, compiler, test.globs) File "/Users/mkoeppe/s/sage/sagerebasing/worktreerebase/.tox/localhomebrewmacosminimal/local/lib/python3.9/sitepackages/sage/doctest/forker.py", line 1137, in compile_and_execute exec(compiled, globs) File "<doctest sage.tests.books.computationalmathematicswithsagemath.calculus_doctest[89]>", line 1, in <module> n0 = find_root(u(n)  RealNumber('1e8') == 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/sagerebasing/worktreerebase/.tox/localhomebrewmacosminimal/local/lib/python3.9/sitepackages/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/sagerebasing/worktreerebase/.tox/localhomebrewmacosminimal/local/lib/python3.9/sitepackages/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 randomseed=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 1e6 Expected: 1.41421356237 Got: 0.0 Tolerance exceeded: 1.41421356237 vs 0.0, tolerance 2e0 > 1e6 ********************************************************************** 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
 Commit changed from a4545f2fc51cd256c16d437a5bc49ae5323ae9e6 to 8446bf39a1eac8e44b57b98e250ccbeb93217c14
Branch pushed to git repo; I updated commit sha1. New commits:
8446bf3  Trac #32234: fix symbolic find_local_minimum() docs.

comment:39 Changed 6 months ago by
 Commit changed from 8446bf39a1eac8e44b57b98e250ccbeb93217c14 to 1796d81e54cbe3bf756927aeaca599511bff5f75
Branch pushed to git repo; I updated commit sha1. New commits:
1796d81  Trac #32234: use domain=float for fast callables used by numpy.

comment:40 Changed 6 months ago by
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
 Cc ghmjungmath 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
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 followup ticket.
comment:43 Changed 6 months ago by
(That's now #32268.)
comment:44 Changed 6 months ago by
 Cc vdelecroix added
comment:45 in reply to: ↑ 35 Changed 6 months ago by
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 fastcallablearg2 stuff on my machine.
comment:46 Changed 6 months ago by
 Commit changed from 1796d81e54cbe3bf756927aeaca599511bff5f75 to df062b30005bf8488fa16124cd7c2b9129e7b396
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 fastcallables in plot3d().

comment:47 Changed 6 months ago by
 Cc ghmwageringel added
comment:48 Changed 5 months ago by
 Cc kcrisman slelievre added
comment:49 Changed 5 months ago by
 Branch changed from u/mjo/ticket/32234 to u/mkoeppe/ticket/32234
comment:50 Changed 5 months ago by
 Commit changed from df062b30005bf8488fa16124cd7c2b9129e7b396 to a4c840df6311ff89434e559fa079f399b354fa18
comment:51 Changed 4 months ago by
 Commit changed from a4c840df6311ff89434e559fa079f399b354fa18 to 72f388109b770a03b370a8730aa666ab95fe0e78
Branch pushed to git repo; I updated commit sha1. New commits:
72f3881  Merge tag '9.5.beta1' into t/32234/ticket/32234

comment:52 Changed 4 months ago by
This ticket needs another reviewer...
comment:53 Changed 4 months ago by
 Cc nbruin added
comment:54 Changed 4 months ago by
 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
That would be fine with me
comment:56 Changed 4 months ago by
 Commit changed from 96ba1213fa281beba55d95f15352b09699eea45f to 6fc96d64d66b7e894fc0703a9324f43d54474bc4
Branch pushed to git repo; I updated commit sha1. New commits:
6fc96d6  Trac #32234: fix pyflakes warning (missing scipy import).

comment:57 Changed 4 months ago by
 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
 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
 Branch changed from u/mjo/ticket/32234 to 6fc96d64d66b7e894fc0703a9324f43d54474bc4
 Resolution set to fixed
 Status changed from positive_review to closed
Cc'ing the last person who modified
numerical_integral
...