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

Fix complex handling of libs/pynac/py_float() #23077

Closed
rwst opened this issue May 25, 2017 · 29 comments
Closed

Fix complex handling of libs/pynac/py_float() #23077

rwst opened this issue May 25, 2017 · 29 comments

Comments

@rwst
Copy link

rwst commented May 25, 2017

The goal is to always return some float, either real or complex but:

sage: from sage.libs.pynac.pynac import py_float_for_doctests as py_float
sage: py_float(I, {'parent':RealField(10)})
...
TypeError: unable to convert '1.0*I' to a real number

The code shows it's only done if there is no parent given:

    if kwds is not NULL:
        return (<object>kwds)['parent'](n)
    else:
        try:
            return RR(n)
        except TypeError:
            return CC(n)

The ticket should make it possible to fall back on the associated complex field of any parent.

Depends on #23145

Component: interfaces

Author: Ralf Stephan

Branch/Commit: 856f8dd

Reviewer: Travis Scrimshaw

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

@rwst rwst added this to the sage-8.0 milestone May 25, 2017
@rwst
Copy link
Author

rwst commented May 25, 2017

@rwst
Copy link
Author

rwst commented May 25, 2017

comment:2

Still one doctest to fix. #18386 depends on this.


New commits:

cb8851323077: Fix complex handling of libs/pynac/py_float()

@rwst
Copy link
Author

rwst commented May 25, 2017

Commit: cb88513

@rwst
Copy link
Author

rwst commented May 26, 2017

comment:3

BTW, the numeric doctest changes have better accuracy, so consider it a bug fix for inaccurate results as well.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 26, 2017

Changed commit from cb88513 to b5c0864

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 26, 2017

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

b5c086423077: fix jacobi_P numerics

@rwst
Copy link
Author

rwst commented May 26, 2017

Author: Ralf Stephan

@tscrim
Copy link
Collaborator

tscrim commented Jun 3, 2017

comment:6

Is it ever (natually) possible for the parent to not have a complex_field method?

@rwst
Copy link
Author

rwst commented Jun 4, 2017

comment:7

Replying to @tscrim:

Is it ever (natually) possible for the parent to not have a complex_field method?

Try RLF.

@tscrim
Copy link
Collaborator

tscrim commented Jun 4, 2017

comment:8

So could we get into a situation where this raises an AttributeError:

diff --git a/src/sage/functions/orthogonal_polys.py b/src/sage/functions/orthogonal_polys.py
index 931b247..51da378 100644
--- a/src/sage/functions/orthogonal_polys.py
+++ b/src/sage/functions/orthogonal_polys.py
@@ -1942,7 +1942,10 @@ class Func_jacobi_P(OrthogonalFunction):
         prec = the_parent.precision()
         BF = CBF(prec+5)
         ret = BF(x).jacobi_P(BF(n), BF(a), BF(b))
-        return the_parent(ret)
+        try:
+            return the_parent(ret)
+        except (TypeError, ValueError):
+            return the_parent.complex_field()(ret)
 
 jacobi_P = Func_jacobi_P()
 

Should we catch the error?

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 5, 2017

Changed commit from b5c0864 to bb56bba

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 5, 2017

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

9b8ab0fMerge branch 'develop' into t/23077/fix_complex_handling_of_libs_pynac_py_float__
bb56bba23077: make complex field handling consistent

@rwst
Copy link
Author

rwst commented Jun 5, 2017

comment:10

I agree this should be consistent and did so. Should there be a ticket for RLF.complex_field() (I'm no algebraist)?

@tscrim
Copy link
Collaborator

tscrim commented Jun 5, 2017

Reviewer: Travis Scrimshaw

@tscrim
Copy link
Collaborator

tscrim commented Jun 5, 2017

comment:11

I'm not sure. However, it is good that we are consistent now. Positive review.

@vbraun
Copy link
Member

vbraun commented Jun 5, 2017

comment:12
[dochtml] [plotting ] File "sage/libs/pynac/pynac.pyx", line 1334, in sage.libs.pynac.pynac.py_float (build/cythonized/sage/libs/pynac/pynac.cpp:15044)
[dochtml] [plotting ] return (<object>kwds)['parent'].complex_field()(n)
[dochtml] [plotting ] AttributeError: type object 'float' has no attribute 'complex_field'
[dochtml] Error building the documentation.
[dochtml] Traceback (most recent call last):
[dochtml]   File "/mnt/disk/home/release/Sage/local/lib/python2.7/runpy.py", line 174, in _run_module_as_main
[dochtml]     "__main__", fname, loader, pkg_name)
[dochtml]   File "/mnt/disk/home/release/Sage/local/lib/python2.7/runpy.py", line 72, in _run_code
[dochtml]     exec code in run_globals
[dochtml]   File "/mnt/disk/home/release/Sage/local/lib/python2.7/site-packages/sage_setup/docbuild/__main__.py", line 2, in <module>
[dochtml]     main()
[dochtml]   File "/mnt/disk/home/release/Sage/local/lib/python2.7/site-packages/sage_setup/docbuild/__init__.py", line 1642, in main
[dochtml]     builder()
[dochtml]   File "/mnt/disk/home/release/Sage/local/lib/python2.7/site-packages/sage_setup/docbuild/__init__.py", line 316, in _wrapper
[dochtml]     getattr(get_builder(document), 'inventory')(*args, **kwds)
[dochtml]   File "/mnt/disk/home/release/Sage/local/lib/python2.7/site-packages/sage_setup/docbuild/__init__.py", line 510, in _wrapper
[dochtml]     build_many(build_ref_doc, L)
[dochtml]   File "/mnt/disk/home/release/Sage/local/lib/python2.7/site-packages/sage_setup/docbuild/__init__.py", line 252, in build_many
[dochtml]     ret = x.get(99999)
[dochtml]   File "/mnt/disk/home/release/Sage/local/lib/python2.7/multiprocessing/pool.py", line 567, in get
[dochtml]     raise self._value
[dochtml] OSError: [plotting ] /mnt/disk/home/release/Sage/local/lib/python2.7/site-packages/sage/plot/plot.py:docstring of sage.plot.plot.plot:459: WARNING: Exception occurred in plotting plot-31

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 6, 2017

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

a85dc3323077: improvements, doctests

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 6, 2017

Changed commit from bb56bba to a85dc33

@rwst
Copy link
Author

rwst commented Jun 6, 2017

comment:14

Also float does not have a complex_field() member. Code changes make a review necessary. Could you please?

@rwst
Copy link
Author

rwst commented Jun 6, 2017

comment:15

Ah no, further repercussions, sorry.

@rwst
Copy link
Author

rwst commented Jun 6, 2017

comment:16

Reason is interval fields do not have a _float_() member---they have n() and I consider this a bug.

@rwst
Copy link
Author

rwst commented Jun 6, 2017

comment:17

Oh, ball fields have it neither. Their elements will all fail conversion to float/complex. Seriously.

@rwst
Copy link
Author

rwst commented Jun 6, 2017

Dependencies: #23145

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 7, 2017

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

8028da523145: Interval and ball field elements conversion to Python float/complex

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 7, 2017

Changed commit from a85dc33 to 8028da5

@vbraun
Copy link
Member

vbraun commented Jun 13, 2017

comment:22
sage -t --long --warn-long 74.0 src/sage/rings/polynomial/polynomial_rational_flint.pyx
**********************************************************************
File "src/sage/rings/polynomial/polynomial_rational_flint.pyx", line 612, in sage.rings.polynomial.polynomial_rational_flint.Polynomial_rational_flint.reverse
Failed example:
    f.reverse(I)
Expected:
    Traceback (most recent call last):
    ...
    ValueError: can not convert complex algebraic number to real interval
Got:
    <BLANKLINE>
    Traceback (most recent call last):
      File "/mnt/disk/home/release/Sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 509, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/mnt/disk/home/release/Sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 872, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.rings.polynomial.polynomial_rational_flint.Polynomial_rational_flint.reverse[17]>", line 1, in <module>
        f.reverse(I)
      File "sage/rings/polynomial/polynomial_rational_flint.pyx", line 632, in sage.rings.polynomial.polynomial_rational_flint.Polynomial_rational_flint.reverse (build/cythonized/sage/rings/polynomial/polynomial_rational_flint.cpp:8018)
        len = <unsigned long> (degree + 1)
      File "sage/symbolic/expression.pyx", line 1104, in sage.symbolic.expression.Expression.__int__ (build/cythonized/sage/symbolic/expression.cpp:9138)
        raise ValueError("cannot convert %s to int" % self)
    ValueError: cannot convert I + 1 to int
**********************************************************************
1 item had failures:
   1 of  20 in sage.rings.polynomial.polynomial_rational_flint.Polynomial_rational_flint.reverse
    [393 tests, 1 failure, 10.22 s]
----------------------------------------------------------------------
sage -t --long --warn-long 74.0 src/sage/rings/polynomial/polynomial_rational_flint.pyx  # 1 doctest failed
----------------------------------------------------------------------
Total time for all tests: 10.3 seconds
    cpu time: 6.3 seconds
    cumulative wall time: 10.2 seconds

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 14, 2017

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

40deeacMerge branch 'develop' into t/23077/fix_complex_handling_of_libs_pynac_py_float__
856f8dd23077: improve error handling/msg

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 14, 2017

Changed commit from 8028da5 to 856f8dd

@vbraun
Copy link
Member

vbraun commented Jun 15, 2017

Changed branch from u/rws/fix_complex_handling_of_libs_pynac_py_float__ to 856f8dd

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

3 participants