Opened 4 years ago

Closed 4 years ago

# complex_root_of uses inexact index

Reported by: Owned by: jdemeyer blocker sage-8.2 symbolics rws, charpent Ralf Stephan Jeroen Demeyer N/A 277a403 277a403ef00692247f9853d89322f9a464abd471

When numerically evaluating `complex_root_of(a, k)`, the index `k` is passed as floating-point number:

```sage: complex_root_of(x^8-1, 7).n(2)
---------------------------------------------------------------------------
IndexError                                Traceback (most recent call last)
<ipython-input-1-4eba84b7c14c> in <module>()
----> 1 complex_root_of(x**Integer(8)-Integer(1), Integer(7)).n(Integer(2))

/home/patchbot/sage-patchbot/src/sage/structure/element.pyx in sage.structure.element.Element.n (build/cythonized/sage/structure/element.c:8131)()
863             0.666666666666667
864         """
--> 865         return self.numerical_approx(prec, digits, algorithm)
866
867     N = deprecated_function_alias(13055, n)

/home/patchbot/sage-patchbot/src/sage/symbolic/expression.pyx in sage.symbolic.expression.Expression.numerical_approx (build/cythonized/sage/symbolic/expression.cpp:36209)()
5818         kwds = {'parent': R, 'algorithm': algorithm}
5819         try:
-> 5820             x = x._convert(kwds)
5821         except TypeError: # numerical approximation for real number failed
5822             pass          # try again with complex

/home/patchbot/sage-patchbot/src/sage/symbolic/expression.pyx in sage.symbolic.expression.Expression._convert (build/cythonized/sage/symbolic/expression.cpp:10663)()
1257         sig_on()
1258         try:
-> 1259             res = self._gobj.evalf(0, kwds)
1260         finally:
1261             sig_off()

/home/patchbot/sage-patchbot/local/lib/python2.7/site-packages/sage/functions/other.pyc in _evalf_(self, poly, index, parent, algorithm)
2918         except AttributeError:
2919             prec = 53
-> 2920         sobj = CRootOf(Poly(poly._sympy_()), int(index))
2921         return sobj.n(ceil(prec*3/10))._sage_()
2922

/home/patchbot/sage-patchbot/local/lib/python2.7/site-packages/sympy/polys/rootoftools.pyc in __new__(cls, f, x, index, radicals, expand)
130         if index < -degree or index >= degree:
131             raise IndexError("root index out of [%d, %d] range, got %d" %
--> 132                              (-degree, degree - 1, index))
133         elif index < 0:
134             index += degree

IndexError: root index out of [-8, 7] range, got 8
```

When `gmpy2` is installed, the index is even passed as complex number:

```sage -t --long src/sage/functions/other.py
**********************************************************************
File "src/sage/functions/other.py", line 2862, in sage.functions.other.Function_crootof
Failed example:
c.n()
Exception raised:
Traceback (most recent call last):
File "/home/patchbot/sage-patchbot/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 517, in _run
self.compile_and_execute(example, compiler, test.globs)
File "/home/patchbot/sage-patchbot/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 920, in compile_and_execute
exec(compiled, globs)
File "<doctest sage.functions.other.Function_crootof[1]>", line 1, in <module>
c.n()
File "sage/structure/element.pyx", line 865, in sage.structure.element.Element.n (build/cythonized/sage/structure/element.c:8131)
return self.numerical_approx(prec, digits, algorithm)
File "sage/symbolic/expression.pyx", line 5824, in sage.symbolic.expression.Expression.numerical_approx (build/cythonized/sage/symbolic/expression.cpp:36286)
x = x._convert(kwds)
File "sage/symbolic/expression.pyx", line 1259, in sage.symbolic.expression.Expression._convert (build/cythonized/sage/symbolic/expression.cpp:10663)
res = self._gobj.evalf(0, kwds)
File "/home/patchbot/sage-patchbot/local/lib/python2.7/site-packages/sage/functions/other.py", line 2920, in _evalf_
sobj = CRootOf(Poly(poly._sympy_()), int(index))
File "sage/rings/complex_number.pyx", line 1058, in sage.rings.complex_number.ComplexNumber.__int__ (build/cythonized/sage/rings/complex_number.c:10918)
raise TypeError("can't convert complex to int; use int(abs(z))")
TypeError: can't convert complex to int; use int(abs(z))
**********************************************************************
File "src/sage/functions/other.py", line 2864, in sage.functions.other.Function_crootof
Failed example:
c.n(100)
Exception raised:
Traceback (most recent call last):
File "/home/patchbot/sage-patchbot/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 517, in _run
self.compile_and_execute(example, compiler, test.globs)
File "/home/patchbot/sage-patchbot/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 920, in compile_and_execute
exec(compiled, globs)
File "<doctest sage.functions.other.Function_crootof[2]>", line 1, in <module>
c.n(Integer(100))
File "sage/structure/element.pyx", line 865, in sage.structure.element.Element.n (build/cythonized/sage/structure/element.c:8131)
return self.numerical_approx(prec, digits, algorithm)
File "sage/symbolic/expression.pyx", line 5824, in sage.symbolic.expression.Expression.numerical_approx (build/cythonized/sage/symbolic/expression.cpp:36286)
x = x._convert(kwds)
File "sage/symbolic/expression.pyx", line 1259, in sage.symbolic.expression.Expression._convert (build/cythonized/sage/symbolic/expression.cpp:10663)
res = self._gobj.evalf(0, kwds)
File "/home/patchbot/sage-patchbot/local/lib/python2.7/site-packages/sage/functions/other.py", line 2920, in _evalf_
sobj = CRootOf(Poly(poly._sympy_()), int(index))
File "sage/rings/complex_number.pyx", line 1058, in sage.rings.complex_number.ComplexNumber.__int__ (build/cythonized/sage/rings/complex_number.c:10918)
raise TypeError("can't convert complex to int; use int(abs(z))")
TypeError: can't convert complex to int; use int(abs(z))
**********************************************************************
File "src/sage/functions/other.py", line 2866, in sage.functions.other.Function_crootof
Failed example:
(c^6 + c + 1).n(100) < 1e-25
Exception raised:
Traceback (most recent call last):
File "/home/patchbot/sage-patchbot/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 517, in _run
self.compile_and_execute(example, compiler, test.globs)
File "/home/patchbot/sage-patchbot/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 920, in compile_and_execute
exec(compiled, globs)
File "<doctest sage.functions.other.Function_crootof[3]>", line 1, in <module>
(c**Integer(6) + c + Integer(1)).n(Integer(100)) < RealNumber('1e-25')
File "sage/structure/element.pyx", line 865, in sage.structure.element.Element.n (build/cythonized/sage/structure/element.c:8131)
return self.numerical_approx(prec, digits, algorithm)
File "sage/symbolic/expression.pyx", line 5824, in sage.symbolic.expression.Expression.numerical_approx (build/cythonized/sage/symbolic/expression.cpp:36286)
x = x._convert(kwds)
File "sage/symbolic/expression.pyx", line 1259, in sage.symbolic.expression.Expression._convert (build/cythonized/sage/symbolic/expression.cpp:10663)
res = self._gobj.evalf(0, kwds)
File "/home/patchbot/sage-patchbot/local/lib/python2.7/site-packages/sage/functions/other.py", line 2920, in _evalf_
sobj = CRootOf(Poly(poly._sympy_()), int(index))
File "sage/rings/complex_number.pyx", line 1058, in sage.rings.complex_number.ComplexNumber.__int__ (build/cythonized/sage/rings/complex_number.c:10918)
raise TypeError("can't convert complex to int; use int(abs(z))")
TypeError: can't convert complex to int; use int(abs(z))
**********************************************************************
File "src/sage/functions/other.py", line 2908, in sage.functions.other.Function_crootof._evalf_
Failed example:
complex_root_of(x^2-2, 1).n()
Exception raised:
Traceback (most recent call last):
File "/home/patchbot/sage-patchbot/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 517, in _run
self.compile_and_execute(example, compiler, test.globs)
File "/home/patchbot/sage-patchbot/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 920, in compile_and_execute
exec(compiled, globs)
File "<doctest sage.functions.other.Function_crootof._evalf_[0]>", line 1, in <module>
complex_root_of(x**Integer(2)-Integer(2), Integer(1)).n()
File "sage/structure/element.pyx", line 865, in sage.structure.element.Element.n (build/cythonized/sage/structure/element.c:8131)
return self.numerical_approx(prec, digits, algorithm)
File "sage/symbolic/expression.pyx", line 5824, in sage.symbolic.expression.Expression.numerical_approx (build/cythonized/sage/symbolic/expression.cpp:36286)
x = x._convert(kwds)
File "sage/symbolic/expression.pyx", line 1259, in sage.symbolic.expression.Expression._convert (build/cythonized/sage/symbolic/expression.cpp:10663)
res = self._gobj.evalf(0, kwds)
File "/home/patchbot/sage-patchbot/local/lib/python2.7/site-packages/sage/functions/other.py", line 2920, in _evalf_
sobj = CRootOf(Poly(poly._sympy_()), int(index))
File "sage/rings/complex_number.pyx", line 1058, in sage.rings.complex_number.ComplexNumber.__int__ (build/cythonized/sage/rings/complex_number.c:10918)
raise TypeError("can't convert complex to int; use int(abs(z))")
TypeError: can't convert complex to int; use int(abs(z))
**********************************************************************
3 of   5 in sage.functions.other.Function_crootof
1 of   3 in sage.functions.other.Function_crootof._evalf_
[622 tests, 4 failures, 17.27 s]
sage -t --long src/sage/interfaces/sympy.py
**********************************************************************
File "src/sage/interfaces/sympy.py", line 617, in sage.interfaces.sympy._sympysage_crootof
Failed example:
(sols[0]+1)._sage_().n()
Exception raised:
Traceback (most recent call last):
File "/home/patchbot/sage-patchbot/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 517, in _run
self.compile_and_execute(example, compiler, test.globs)
File "/home/patchbot/sage-patchbot/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 920, in compile_and_execute
exec(compiled, globs)
File "<doctest sage.interfaces.sympy._sympysage_crootof[6]>", line 1, in <module>
(sols[Integer(0)]+Integer(1))._sage_().n()
File "sage/structure/element.pyx", line 865, in sage.structure.element.Element.n (build/cythonized/sage/structure/element.c:8131)
return self.numerical_approx(prec, digits, algorithm)
File "sage/symbolic/expression.pyx", line 5824, in sage.symbolic.expression.Expression.numerical_approx (build/cythonized/sage/symbolic/expression.cpp:36286)
x = x._convert(kwds)
File "sage/symbolic/expression.pyx", line 1259, in sage.symbolic.expression.Expression._convert (build/cythonized/sage/symbolic/expression.cpp:10663)
res = self._gobj.evalf(0, kwds)
File "/home/patchbot/sage-patchbot/local/lib/python2.7/site-packages/sage/functions/other.py", line 2920, in _evalf_
sobj = CRootOf(Poly(poly._sympy_()), int(index))
File "sage/rings/complex_number.pyx", line 1058, in sage.rings.complex_number.ComplexNumber.__int__ (build/cythonized/sage/rings/complex_number.c:10918)
raise TypeError("can't convert complex to int; use int(abs(z))")
TypeError: can't convert complex to int; use int(abs(z))
**********************************************************************
1 of   8 in sage.interfaces.sympy._sympysage_crootof
[218 tests, 1 failure, 3.27 s]
```

### comment:2 Changed 4 years ago by jdemeyer

• Description modified (diff)
• Summary changed from Doctest failures related to sympy to Doctest failures related to sympy CRootOf

### comment:3 Changed 4 years ago by rws

• Branch set to u/rws/doctest_failures_related_to_sympy_crootof

### comment:4 follow-ups: ↓ 6 ↓ 10 Changed 4 years ago by rws

• Authors set to Ralf Stephan
• Commit set to a93662faed523eb117f4ce0197b3178855849fd2
• Status changed from new to needs_review

That exact arguments are made inexact is a bug in itself. But that they are made complex (EDIT: in some machines) is beyond me.

New commits:

 ​a93662f `24378: Doctest failures related to sympy`
Last edited 4 years ago by rws (previous) (diff)

### comment:5 Changed 4 years ago by jdemeyer

Do you have an idea why this depends on the system?

### comment:6 in reply to: ↑ 4 Changed 4 years ago by jdemeyer

That exact arguments are made inexact is a bug in itself. But that they are made complex (EDIT: in some machines) is beyond me.

Do you know where this happens? It seems to me that it is Pynac doing this.

### comment:7 Changed 4 years ago by jdemeyer

• Description modified (diff)

### comment:8 Changed 4 years ago by vdelecroix

Some sympy behavior depends on the presence/absence of gmpy2. We had this trouble before the upgrade.

Last edited 4 years ago by vdelecroix (previous) (diff)

### comment:9 Changed 4 years ago by jdemeyer

• Description modified (diff)
• Summary changed from Doctest failures related to sympy CRootOf to complex_root_of uses inexact index

### comment:10 in reply to: ↑ 4 ; follow-up: ↓ 15 Changed 4 years ago by jdemeyer

That exact arguments are made inexact is a bug in itself.

Right, this ticket should just fix that.

### comment:11 Changed 4 years ago by jdemeyer

• Status changed from needs_review to needs_work

### comment:12 Changed 4 years ago by jdemeyer

• Description modified (diff)

### comment:13 Changed 4 years ago by jdemeyer

• Description modified (diff)

### comment:14 Changed 4 years ago by jdemeyer

• Description modified (diff)

### comment:15 in reply to: ↑ 10 Changed 4 years ago by vdelecroix

That exact arguments are made inexact is a bug in itself.

Right, this ticket should just fix that.

Indeed `numerical_approximation` of symbolic expression is lazily converting all arguments to the same parent. Which is very wrong when a function is defined on `R x N`.

Perhaps it would be good to add in `_evalf_` the check

```if not isinstance(index, numbers.Integral):
raise ValueError
```

Last edited 4 years ago by vdelecroix (previous) (diff)

### comment:16 Changed 4 years ago by vdelecroix

Indeed there is a sympy bug that is now tracked at #24380.

### comment:17 follow-up: ↓ 21 Changed 4 years ago by vdelecroix

`numerical_approx` is very wrong when evaluating. It converts `x^2 - 2` to `x^2 - 2.0` and hence loosing precision for no good reason.

### comment:18 Changed 4 years ago by vdelecroix

There was indeed a bug in sympy: #24380 needs review!

### comment:19 Changed 4 years ago by rws

I suggest accepting my quick fix to resolve the blocker. Meanwhile I'm looking into Pynac.

### comment:20 Changed 4 years ago by jdemeyer

If your only goal is to resolve the blocker doctest failures, then I think it is better to use `# known bug`. The fact that the index is made floating-point is the real bug, whether it is real or complex is just a small variation.

### comment:21 in reply to: ↑ 17 ; follow-up: ↓ 22 Changed 4 years ago by rws

`numerical_approx` is very wrong when evaluating. It converts `x^2 - 2` to `x^2 - 2.0` and hence loosing precision for no good reason.

I disagree in this case. The result is exactly what was asked for.

### comment:22 in reply to: ↑ 21 ; follow-ups: ↓ 23 ↓ 24 Changed 4 years ago by vdelecroix

`numerical_approx` is very wrong when evaluating. It converts `x^2 - 2` to `x^2 - 2.0` and hence loosing precision for no good reason.

I disagree in this case. The result is exactly what was asked for.

The problem under discussion is that you have a number specified by an equation like `x^2 - 2 = 0`. Now, you want an numerical approximation of it. I claim that there is not much reason to transform the equation into `1.0 * x^2 - 2.0` for doing that. In particular, this is not what the user wants. This kind of conversion is a can of worms. If you want 100 bits of your number, and start by converting the coefficients to floating points with 100 bits of precision you might loose your roots.

Note that in sympy they solve the original equation with appropriate root finding algorithms on rational numbers.

### comment:23 in reply to: ↑ 22 Changed 4 years ago by rws

`numerical_approx` is very wrong when evaluating. It converts `x^2 - 2` to `x^2 - 2.0` and hence loosing precision for no good reason.

I disagree in this case. The result is exactly what was asked for.

The problem under discussion is that you have a number specified by an equation like `x^2 - 2 = 0`. Now, you want an numerical approximation of it.

No, that is solving. `N()` just converts symbolic expressions---if they happen to contain symbols you won't get a number, you get an error. `_convert()` helps with that.

Note that in sympy they solve the original equation with appropriate root finding algorithms on rational numbers.

This has nothing to do with this ticket. This ticket is about losing precision in expressions that can be FP evaluated because they contain no symbols but may contain functions. While the result precision from the function at hand `complex_root_of` is in principle not affected by the exactness of the index there may be existing/future functions where it is. This can be fixed by not FP converting exact function arguments and letting the function itself (i.e. its `evalf` member) decide what to do with exact arguments. I may well be wrong, and the best solution instead is a type database for function arguments...

### comment:24 in reply to: ↑ 22 Changed 4 years ago by rws

Note that in sympy they solve the original equation with appropriate root finding algorithms on rational numbers.

Yes but we don't do the solving. We let SymPy do it.

### comment:25 Changed 4 years ago by rws

• Branch changed from u/rws/doctest_failures_related_to_sympy_crootof to u/rws/24378

### comment:26 Changed 4 years ago by rws

• Branch changed from u/rws/24378 to u/rws/doctest_failures_related_to_sympy_crootof
• Status changed from needs_work to needs_review

### comment:27 Changed 4 years ago by rws

• Branch changed from u/rws/doctest_failures_related_to_sympy_crootof to u/rws/24378
• Commit changed from a93662faed523eb117f4ce0197b3178855849fd2 to ac7681458cdeb8b6118080f81f3d9601db485021

New commits:

 ​ac76814 `24378: avoid doctest fails`

### comment:28 Changed 4 years ago by rws

See #24397 for the fix.

### comment:29 Changed 4 years ago by jdemeyer

• Status changed from needs_review to needs_work

You should add the ticket number: `# known bug (Trac #24397)`.

### comment:30 Changed 4 years ago by git

• Commit changed from ac7681458cdeb8b6118080f81f3d9601db485021 to 49c6d614b4293059911f19ccedb87a3c5e96df35

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

 ​49c6d61 `24378: cosmetics`

### comment:31 Changed 4 years ago by rws

• Status changed from needs_work to needs_review

### comment:32 Changed 4 years ago by rws

• Branch changed from u/rws/24378 to u/rws/24378-1

### comment:33 Changed 4 years ago by rws

• Commit changed from 49c6d614b4293059911f19ccedb87a3c5e96df35 to ae3a1e0be21a0aecbb46791fc2cacf84495eee5c

Fortunately this can be resolved easily.

New commits:

 ​ae3a1e0 `24387: complex_root_of uses inexact index`

### comment:34 Changed 4 years ago by rws

I have added the information to https://github.com/pynac/pynac/wiki/%7C-floating-point-evaluation, but Sage `Function` initialization options need better docs too.

### comment:35 Changed 4 years ago by jdemeyer

• Status changed from needs_review to needs_work

If this fixes the actual underlying issue, could you add

```sage: complex_root_of(x^8-1, 7).n(2)
```

as a doctest?

### comment:36 Changed 4 years ago by rws

It fixes the issue but there are other problems at super-low precision.

### comment:37 Changed 4 years ago by git

• Commit changed from ae3a1e0be21a0aecbb46791fc2cacf84495eee5c to 8c3f951c7b5f726260ccadb74368713d90740d93

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

 ​8c3f951 `24378: set minimum digits; convert to given prec`

### comment:38 Changed 4 years ago by rws

• Status changed from needs_work to needs_review

### comment:39 Changed 4 years ago by jdemeyer

• Reviewers set to Jeroen Demeyer
• Status changed from needs_review to needs_work

The expression `prec*3/10` will behave differently on Python 2 and Python 3. You should either use `//` or `from __future__ import division` such that it works the same on Python 2 and Python 3.

### comment:40 follow-up: ↓ 41 Changed 4 years ago by jdemeyer

What is the meaning of the factor `3/10` anyway?

### comment:41 in reply to: ↑ 40 ; follow-up: ↓ 42 Changed 4 years ago by rws

What is the meaning of the factor `3/10` anyway?

An approximation to `log(10,2)` which is good enough until about 1000 bits precision.

Version 0, edited 4 years ago by rws (next)

### comment:42 in reply to: ↑ 41 Changed 4 years ago by jdemeyer

What is the meaning of the factor `3/10` anyway?

An approximation to `log(2,10)` (EDITED) which is good enough until about 1000 bits precision.

Then why don't use the actual value `0.3010299956639812`? That avoids the problem with division also.

### comment:43 Changed 4 years ago by git

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

 ​8e65608 `24378: don't prematurely optimize`

### comment:44 Changed 4 years ago by rws

• Status changed from needs_work to needs_review

Ping?

### comment:46 Changed 4 years ago by vdelecroix

`ceil(prec*log(2,10)))` is terrible. Why not using `0.3010299956639812` as suggested in comment:42? A possible alternative would be

```from math import log10
int(prec * log10(2))
```

You definitely do not want to use two symbolic functions here.

### comment:47 Changed 4 years ago by git

• Commit changed from 8e65608d35c2527bcf6a7ebba8eadc7eb0377c38 to 14ed7c6f210f4e809c3458dc54315542bbf3547b

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

 ​14ed7c6 `24378: fix previous commit`

### comment:48 Changed 4 years ago by jdemeyer

Or better yet, use the sympy function `prec_to_dps` which is meant to convert bits to digits:

```from sympy.core.evalf import prec_to_dps
```

### comment:49 Changed 4 years ago by rws

Thanks both. I'll make a fresh branch.

### comment:50 Changed 4 years ago by rws

• Branch changed from u/rws/24378-1 to u/rws/24378-2

### comment:51 Changed 4 years ago by rws

• Commit changed from 14ed7c6f210f4e809c3458dc54315542bbf3547b to 277a403ef00692247f9853d89322f9a464abd471

A minor note is that the results from `prec_dps(n)` are not identical to the previously proposed formulae at small values, even with a suitable shift.

New commits:

 ​277a403 `24378: complex_root_of uses inexact index`

### comment:52 Changed 4 years ago by jdemeyer

Good for me if this passes patchbot testing.

### comment:53 Changed 4 years ago by rws

• Status changed from needs_review to positive_review

### comment:54 Changed 4 years ago by vbraun

• Branch changed from u/rws/24378-2 to 277a403ef00692247f9853d89322f9a464abd471
• Resolution set to fixed
• Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.