Opened 3 years ago
Closed 3 years ago
#24630 closed defect (fixed)
Inconsistency in conversion from CIF and complex
Reported by:  rws  Owned by:  

Priority:  major  Milestone:  sage8.2 
Component:  numerical  Keywords:  
Cc:  Merged in:  
Authors:  Ralf Stephan  Reviewers:  Jeroen Demeyer 
Report Upstream:  N/A  Work issues:  
Branch:  7b23595 (Commits, GitHub, GitLab)  Commit:  7b23595d11b6839812558066889851bd39f56ac0 
Dependencies:  Stopgaps: 
Description (last modified by )
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))`.
Change History (16)
comment:1 Changed 3 years ago by
 Description modified (diff)
 Summary changed from Inconsistency in conversion from CIF to Inconsistency in conversion from CIF and complex
comment:2 Changed 3 years ago by
 Branch set to u/rws/inconsistency_in_conversion_from_cif_and_complex
comment:3 Changed 3 years ago by
 Commit set to 1218a8aa020ce5714695eb2f78beda09bc7a5aa9
 Status changed from new to needs_review
comment:4 Changed 3 years ago by
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)
.
comment:5 Changed 3 years ago by
 Commit changed from 1218a8aa020ce5714695eb2f78beda09bc7a5aa9 to 3a4183724b88be1150d6564e6da5dba10653ff0f
comment:6 Changed 3 years ago by
 Status changed from needs_review to needs_work
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?
comment:7 followup: ↓ 8 Changed 3 years ago by
Is this issue fixed by #24371? (Also, this is to warn about a possible merge conflict.)
comment:8 in reply to: ↑ 7 Changed 3 years ago by
comment:9 Changed 3 years ago by
 Status changed from needs_work to needs_review
comment:10 Changed 3 years ago by
 Status changed from needs_review to needs_work
comment:11 Changed 3 years ago by
 Commit changed from 3a4183724b88be1150d6564e6da5dba10653ff0f to 2796d1aa6174c4092359b8a189bf8bab2ce5e8ba
comment:12 Changed 3 years ago by
 Status changed from needs_work to needs_review
I just saw I didn't address your last comment, sorry.
comment:13 Changed 3 years ago by
 Status changed from needs_review to needs_work
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.
comment:14 Changed 3 years ago by
 Commit changed from 2796d1aa6174c4092359b8a189bf8bab2ce5e8ba to 7b23595d11b6839812558066889851bd39f56ac0
Branch pushed to git repo; I updated commit sha1. New commits:
7b23595  24630: address reviewer's comments

comment:15 Changed 3 years ago by
 Reviewers set to Jeroen Demeyer
 Status changed from needs_work to positive_review
Since patchbot is okay, I'll take this as positive?
comment:16 Changed 3 years ago by
 Branch changed from u/rws/inconsistency_in_conversion_from_cif_and_complex to 7b23595d11b6839812558066889851bd39f56ac0
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
24630: conversions CIF>real, Python complex>real