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

Inconsistency in conversion from CIF and complex #24630

Closed
rwst opened this issue Feb 1, 2018 · 20 comments
Closed

Inconsistency in conversion from CIF and complex #24630

rwst opened this issue Feb 1, 2018 · 20 comments

Comments

@rwst
Copy link

rwst commented Feb 1, 2018

sage: RR(CBF(1.7))
1.70000000000000

sage: CIF(1.7).is_exact()
True
sage: RR(RIF(1.7))
1.70000000000000
sage: RR(CIF(1.7))

/home/ralf/sage/src/sage/structure/parent.pyx in sage.structure.parent.Parent.__call__ (build/cythonized/sage/structure/parent.c:9530)()
    916         if mor is not None:
    917             if no_extra_args:
--> 918                 return mor._call_(x)
    919             else:
    920                 return mor._call_with_args(x, args, kwds)

/home/ralf/sage/src/sage/structure/coerce_maps.pyx in sage.structure.coerce_maps.DefaultConvertMap_unique._call_ (build/cythonized/sage/structure/coerce_maps.c:4979)()
    153                 print(type(C), C)
    154                 print(type(C._element_constructor), C._element_constructor)
--> 155             raise
    156 
    157     cpdef Element _call_with_args(self, x, args=(), kwds={}):

/home/ralf/sage/src/sage/structure/coerce_maps.pyx in sage.structure.coerce_maps.DefaultConvertMap_unique._call_ (build/cythonized/sage/structure/coerce_maps.c:4841)()
    148         cdef Parent C = self._codomain
    149         try:
--> 150             return C._element_constructor(x)
    151         except Exception:
    152             if print_warnings:

/home/ralf/sage/src/sage/rings/real_mpfr.pyx in sage.rings.real_mpfr.RealField_class._element_constructor_ (build/cythonized/sage/rings/real_mpfr.c:7305)()
    643         cdef RealNumber z
    644         z = self._new()
--> 645         z._set(x, base)
    646         return z
    647 

/home/ralf/sage/src/sage/rings/real_mpfr.pyx in sage.rings.real_mpfr.RealNumber._set (build/cythonized/sage/rings/real_mpfr.c:12976)()
   1466                 mpfr_set_inf(self.value, -1)
   1467             else:
-> 1468                 raise TypeError("unable to convert {!r} to a real number".format(s))
   1469 
   1470     cdef _set_from_GEN_REAL(self, GEN g):

TypeError: unable to convert '1.7000000000000000?' to a real number

Same with `RR(complex(1.7))`.

Component: numerical

Author: Ralf Stephan

Branch/Commit: 7b23595

Reviewer: Jeroen Demeyer

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

@rwst rwst added this to the sage-8.2 milestone Feb 1, 2018
@rwst

This comment has been minimized.

@rwst rwst changed the title Inconsistency in conversion from CIF Inconsistency in conversion from CIF and complex Feb 1, 2018
@rwst
Copy link
Author

rwst commented Feb 1, 2018

@rwst
Copy link
Author

rwst commented Feb 1, 2018

Commit: 1218a8a

@rwst
Copy link
Author

rwst commented Feb 1, 2018

New commits:

1218a8a24630: conversions CIF-->real, Python complex-->real

@rwst
Copy link
Author

rwst commented Feb 1, 2018

Author: Ralf Stephan

@mezzarobba
Copy link
Member

comment:4

Mostly looks good to me, but I don't understand why you are using parent(x) is complex instead of type(x) is complex or isinstance(x, complex).

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 14, 2018

Changed commit from 1218a8a to 3a41837

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 14, 2018

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

cf90ca2Merge branch 'develop' into t/24630/inconsistency_in_conversion_from_cif_and_complex
3a4183724630: address reviewer's comment

@mezzarobba
Copy link
Member

comment:6

You no longer need to import parent (this should probably have been a cimport anyhow). Oh, and I missed that last time (sorry), but why are you changing RealField_class._element_constructor_() instead of RealNumber._new() — which apparently contains the logic for most other cases?

@tscrim
Copy link
Collaborator

tscrim commented Feb 15, 2018

comment:7

Is this issue fixed by #24371? (Also, this is to warn about a possible merge conflict.)

@rwst
Copy link
Author

rwst commented Feb 19, 2018

comment:8

Replying to @tscrim:

Is this issue fixed by #24371? (Also, this is to warn about a possible merge conflict.)

No, the same errors are raised.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 19, 2018

Changed commit from 3a41837 to 2796d1a

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 19, 2018

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

9e5072cMerge branch 'develop' into t/24630/inconsistency_in_conversion_from_cif_and_complex
dc476b0Merge branch 'develop' into t/24630/inconsistency_in_conversion_from_cif_and_complex
2796d1a24630: address reviewer's comments

@rwst
Copy link
Author

rwst commented Mar 19, 2018

comment:12

I just saw I didn't address your last comment, sorry.

@jdemeyer
Copy link

comment:13

Can you change the exception to

raise TypeError("f"unable to convert complex interval {self} to real number")

(just trying to be consistent and using the same wording as similar exception messages)

Also, I don't like the "Try to" in the docs. Maybe write If the imaginary part is zero, ...

Apart from that, this looks good to me.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 20, 2018

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

7b2359524630: address reviewer's comments

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 20, 2018

Changed commit from 2796d1a to 7b23595

@rwst
Copy link
Author

rwst commented Mar 20, 2018

comment:15

Since patchbot is okay, I'll take this as positive?

@rwst
Copy link
Author

rwst commented Mar 20, 2018

Reviewer: Jeroen Demeyer

@vbraun
Copy link
Member

vbraun commented Mar 21, 2018

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

5 participants