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: | 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: |
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: | sage-9.4 → sage-9.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 follow-up: 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 (2007-12-07) 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 follow-up: 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 follow-up: 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 --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 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: | gh-mjungmath 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 follow-up 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 fast-callable-arg2 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 fast-callables in plot3d().
|
comment:47 Changed 18 months ago by
Cc: | gh-mwageringel 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
...