Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

complex_root_of uses inexact index #24378

Closed
jdemeyer opened this issue Dec 15, 2017 · 59 comments
Closed

complex_root_of uses inexact index #24378

jdemeyer opened this issue Dec 15, 2017 · 59 comments

Comments

@jdemeyer
Copy link

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))
**********************************************************************
2 items had failures:
   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 item had failures:
   1 of   8 in sage.interfaces.sympy._sympysage_crootof
    [218 tests, 1 failure, 3.27 s]

CC: @rwst @EmmanuelCharpentier

Component: symbolics

Author: Ralf Stephan

Branch/Commit: 277a403

Reviewer: Jeroen Demeyer

Issue created by migration from https://trac.sagemath.org/ticket/24378

@jdemeyer jdemeyer added this to the sage-8.2 milestone Dec 15, 2017
@jdemeyer

This comment has been minimized.

@jdemeyer jdemeyer changed the title Doctest failures related to sympy Doctest failures related to sympy CRootOf Dec 15, 2017
@rwst
Copy link

rwst commented Dec 15, 2017

@rwst
Copy link

rwst commented Dec 15, 2017

comment:4

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:

a93662f24378: Doctest failures related to sympy

@rwst
Copy link

rwst commented Dec 15, 2017

Commit: a93662f

@rwst
Copy link

rwst commented Dec 15, 2017

Author: Ralf Stephan

@jdemeyer
Copy link
Author

comment:5

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

@jdemeyer
Copy link
Author

comment:6

Replying to @rwst:

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.

@jdemeyer

This comment has been minimized.

@videlec
Copy link
Contributor

videlec commented Dec 15, 2017

comment:8

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

@jdemeyer

This comment has been minimized.

@jdemeyer jdemeyer changed the title Doctest failures related to sympy CRootOf complex_root_of uses inexact index Dec 15, 2017
@jdemeyer
Copy link
Author

comment:10

Replying to @rwst:

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

Right, this ticket should just fix that.

@jdemeyer

This comment has been minimized.

@jdemeyer

This comment has been minimized.

@jdemeyer

This comment has been minimized.

@videlec
Copy link
Contributor

videlec commented Dec 15, 2017

comment:15

Replying to @jdemeyer:

Replying to @rwst:

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

Though I am curious about this sympy misbehavior.

@videlec
Copy link
Contributor

videlec commented Dec 15, 2017

comment:16

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

@videlec
Copy link
Contributor

videlec commented Dec 15, 2017

comment:17

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.

@videlec
Copy link
Contributor

videlec commented Dec 15, 2017

comment:18

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

@rwst
Copy link

rwst commented Dec 16, 2017

comment:19

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

@jdemeyer
Copy link
Author

comment:20

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.

@rwst
Copy link

rwst commented Dec 16, 2017

comment:21

Replying to @videlec:

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.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 18, 2017

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

8c3f95124378: set minimum digits; convert to given prec

@jdemeyer
Copy link
Author

comment:39

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.

@jdemeyer
Copy link
Author

Reviewer: Jeroen Demeyer

@jdemeyer
Copy link
Author

comment:40

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

@rwst
Copy link

rwst commented Dec 18, 2017

comment:41

Replying to @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.

@jdemeyer
Copy link
Author

comment:42

Replying to @rwst:

Replying to @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.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 18, 2017

Changed commit from 8c3f951 to 8e65608

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 18, 2017

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

8e6560824378: don't prematurely optimize

@rwst
Copy link

rwst commented Dec 20, 2017

comment:45

Ping?

@videlec
Copy link
Contributor

videlec commented Dec 21, 2017

comment:46

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.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 22, 2017

Changed commit from 8e65608 to 14ed7c6

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 22, 2017

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

14ed7c624378: fix previous commit

@jdemeyer
Copy link
Author

comment:48

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

@rwst
Copy link

rwst commented Dec 23, 2017

comment:49

Thanks both. I'll make a fresh branch.

@rwst
Copy link

rwst commented Dec 23, 2017

Changed branch from u/rws/24378-1 to u/rws/24378-2

@rwst
Copy link

rwst commented Dec 23, 2017

Changed commit from 14ed7c6 to 277a403

@rwst
Copy link

rwst commented Dec 23, 2017

comment:51

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:

277a40324378: complex_root_of uses inexact index

@jdemeyer
Copy link
Author

comment:52

Good for me if this passes patchbot testing.

@vbraun
Copy link
Member

vbraun commented Dec 26, 2017

Changed branch from u/rws/24378-2 to 277a403

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants