Opened 19 months ago
Closed 16 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 19 months ago by
Description:  modified (diff) 

comment:2 Changed 19 months ago by
Description:  modified (diff) 

comment:3 Changed 19 months ago by
Description:  modified (diff) 

comment:4 Changed 19 months ago by
Cc:  chapoton added 

comment:5 Changed 19 months ago by
Branch:  → u/mkoeppe/remove_code_for_fast_float_old_true_ 

comment:6 Changed 19 months ago by
Milestone:  sage9.4 → sage9.5 

comment:7 Changed 19 months ago by
Commit:  → d900e6e4a466fce9e2759ed15b076a47cee168d2 

Needs review, or are you waiting to see if the three uses of _fast_float_
can be removed?
comment:8 Changed 19 months ago by
Authors:  → Matthias Koeppe 

Status:  new → 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 19 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 19 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 Changed 19 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 19 months ago by
Authors:  Matthias Koeppe → Matthias Koeppe, Michael Orlitzky 

Branch:  u/mkoeppe/remove_code_for_fast_float_old_true_ → u/mjo/ticket/32234 
Commit:  d900e6e4a466fce9e2759ed15b076a47cee168d2 → 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 19 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 19 months ago by
Branch:  u/mjo/ticket/32234 → u/mkoeppe/ticket/32234 

comment:15 Changed 19 months ago by
Commit:  010bef3f1dece6aa3a3a5177b2523820caaf072a → 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 Changed 19 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 19 months ago by
Commit:  6fd902cd5f76d2c114f45167712ee521865371de → 9b95060539539269831024d30480b0bea2b4e114 

Branch pushed to git repo; I updated commit sha1. New commits:
9b95060  Remove more _fast_float_ methods

comment:18 Changed 19 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 19 months ago by
Commit:  9b95060539539269831024d30480b0bea2b4e114 → 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 19 months ago by
Commit:  6fc73e8be811568f9c8511f6af6ea75078e1a7ac → 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 19 months ago by
Commit:  b04bdbe49994096e68584567cca8241f0de29f04 → 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 19 months ago by
Commit:  927d6c2ef5b6b0686904d4a207bc2aeca6cd2c26 → 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 19 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 19 months ago by
I'm referring to:
src/sage/numerical/optimize.py: from sage.ext.fast_eval import fast_callable
comment:25 Changed 19 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:28 Changed 19 months ago by
Branch:  u/mkoeppe/ticket/32234 → u/mjo/ticket/32234 

Commit:  6b44072ee77ca7bc5fe276b9d54bcf37662c0218 → 2080a9c3bfd3af58c5295075853c3a69c99e2382 
comment:29 Changed 19 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 19 months ago by
Description:  modified (diff) 

comment:31 Changed 19 months ago by
Commit:  2080a9c3bfd3af58c5295075853c3a69c99e2382 → f3d26be964e76deccbdb51758e0739a4506ac345 

Branch pushed to git repo; I updated commit sha1. New commits:
f3d26be  Trac #32234: replace fast_float_arg() in plot3d.

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

Branch pushed to git repo; I updated commit sha1. New commits:
8446bf3  Trac #32234: fix symbolic find_local_minimum() docs.

comment:39 Changed 19 months ago by
Commit:  8446bf39a1eac8e44b57b98e250ccbeb93217c14 → 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 19 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 19 months ago by
Cc:  ghmjungmath egourgoulhon added 

Reviewers:  → 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 19 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:44 Changed 19 months ago by
Cc:  vdelecroix added 

comment:45 Changed 19 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 18 months ago by
Commit:  1796d81e54cbe3bf756927aeaca599511bff5f75 → 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 18 months ago by
Cc:  ghmwageringel added 

comment:48 Changed 18 months ago by
Cc:  kcrisman slelievre added 

comment:49 Changed 18 months ago by
Branch:  u/mjo/ticket/32234 → u/mkoeppe/ticket/32234 

comment:50 Changed 18 months ago by
Commit:  df062b30005bf8488fa16124cd7c2b9129e7b396 → a4c840df6311ff89434e559fa079f399b354fa18 

comment:51 Changed 17 months ago by
Commit:  a4c840df6311ff89434e559fa079f399b354fa18 → 72f388109b770a03b370a8730aa666ab95fe0e78 

Branch pushed to git repo; I updated commit sha1. New commits:
72f3881  Merge tag '9.5.beta1' into t/32234/ticket/32234

comment:53 Changed 16 months ago by
Cc:  nbruin added 

comment:54 Changed 16 months ago by
Branch:  u/mkoeppe/ticket/32234 → u/mjo/ticket/32234 

Commit:  72f388109b770a03b370a8730aa666ab95fe0e78 → 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:56 Changed 16 months ago by
Commit:  96ba1213fa281beba55d95f15352b09699eea45f → 6fc96d64d66b7e894fc0703a9324f43d54474bc4 

Branch pushed to git repo; I updated commit sha1. New commits:
6fc96d6  Trac #32234: fix pyflakes warning (missing scipy import).

comment:57 Changed 16 months ago by
Reviewers:  Matthias Koeppe, ... → 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 16 months ago by
Reviewers:  Matthias Koeppe, Michael Orlitzky → Michael Orlitzky, Matthias Koeppe 

Status:  needs_review → positive_review 
Then let's get this in.
comment:59 Changed 16 months ago by
Branch:  u/mjo/ticket/32234 → 6fc96d64d66b7e894fc0703a9324f43d54474bc4 

Resolution:  → fixed 
Status:  positive_review → closed 
Cc'ing the last person who modified
numerical_integral
...