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: sage-8.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:

Status badges

Description (last modified by rws)

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 rws

  • 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 rws

  • Branch set to u/rws/inconsistency_in_conversion_from_cif_and_complex

comment:3 Changed 3 years ago by rws

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

New commits:

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

comment:4 Changed 3 years ago by mmezzarobba

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 git

  • Commit changed from 1218a8aa020ce5714695eb2f78beda09bc7a5aa9 to 3a4183724b88be1150d6564e6da5dba10653ff0f

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

comment:6 Changed 3 years ago by mmezzarobba

  • 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 follow-up: Changed 3 years ago by tscrim

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 rws

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.

comment:9 Changed 3 years ago by rws

  • Status changed from needs_work to needs_review

comment:10 Changed 3 years ago by rws

  • Status changed from needs_review to needs_work

comment:11 Changed 3 years ago by git

  • Commit changed from 3a4183724b88be1150d6564e6da5dba10653ff0f to 2796d1aa6174c4092359b8a189bf8bab2ce5e8ba

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

comment:12 Changed 3 years ago by rws

  • 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 jdemeyer

  • 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.

Last edited 3 years ago by jdemeyer (previous) (diff)

comment:14 Changed 3 years ago by git

  • Commit changed from 2796d1aa6174c4092359b8a189bf8bab2ce5e8ba to 7b23595d11b6839812558066889851bd39f56ac0

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

7b2359524630: address reviewer's comments

comment:15 Changed 3 years ago by rws

  • 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 vbraun

  • 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
Note: See TracTickets for help on using tickets.