Opened 11 years ago

Closed 11 years ago

#8869 closed defect (fixed)

float(CDF(1)) should return 1.0, not throw an error

Reported by: jason Owned by: AlexGhitza
Priority: major Milestone: sage-4.4.4
Component: basic arithmetic Keywords: CDF conversion, complex double
Cc: Merged in: sage-4.4.4.alpha0
Authors: Jason Grout Reviewers: Karl-Dieter Crisman, Leif Leonhardy
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description

Right now, we have the following behavior:

sage: float(CC(1.0))
1.0


sage: float(CDF(1.0))
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)

/home/jason/<ipython console> in <module>()

/home/jason/sage/local/lib/python2.6/site-packages/sage/rings/complex_double.so in sage.rings.complex_double.ComplexDoubleElement.__float__ (sage/rings/complex_double.c:6532)()

TypeError: can't convert complex to float; use abs(z)


sage: float(complex(1.0))
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)

/home/jason/<ipython console> in <module>()

TypeError: can't convert complex to float 

As robertwb and was voted (on http://trac.sagemath.org/sage_trac/ticket/5400#comment:12 and on sage-devel), we should make float conversion succeed if the imaginary part is zero.

Attachments (2)

complex-float.patch (3.4 KB) - added by jason 11 years ago.
trac_8869.patch (3.6 KB) - added by kcrisman 11 years ago.
Based on 4.4.2, apply only this patch

Download all attachments as: .zip

Change History (11)

Changed 11 years ago by jason

comment:1 Changed 11 years ago by jason

  • Status changed from new to needs_work

comment:2 Changed 11 years ago by jason

The patch needs to have commit message, and doctests need to be run.

comment:3 Changed 11 years ago by leif

See also http://groups.google.com/group/sage-devel/browse_thread/thread/75b8f85d22499ceb#

(I don't like the use of Python conversion functions on Sage objects.)

Why (only) suggest use of abs()? What about real_part()? Or even imag_part() and arg(), perhaps norm(), too?

Is abs() really more natural than real_part()?

comment:4 Changed 11 years ago by kcrisman

  • Authors set to Jason Grout
  • Reviewers set to Karl-Dieter Crisman, Leif Leonhardy
  • Status changed from needs_work to needs_review

Ready for review. Leif's comment seems reasonable, so I added one (!) extra option in the error message. Passes tests on these two files.

Changed 11 years ago by kcrisman

Based on 4.4.2, apply only this patch

comment:5 Changed 11 years ago by leif

Well, __long__() could equally well succeed if the fractional (and imaginary) part is zero... ;-)

(And note that int(1.1) silently truncates; i.e. the current situation is overall not very consistent, as I mentioned in the thread.)

Nevertheless, I'll test it as soon as the "normal" 4.4.3.alpha0 ptestlong finishes on my Pentium 4, just wait a few hours... ;-)

comment:6 Changed 11 years ago by kcrisman

I don't think we are trying to be contentious here. Yes, there are inconsistencies, but that is just to be expected (I would even say it follows from Arrow's Theorem). The point is to make it as natural to mathematicians as possible, and float(CDF(1)) certainly smells like 1.0 to me. int is a little different, but it seems to me that since Python isn't consistent anyways

>>> int(1.1)
1
>>> float(1+0j)
Traceback (most recent call last):
  File "<stdin>", line 1, in ?
TypeError: can't convert complex to float; use abs(z)

we might as well make the best of it and let int be the "round closest to zero" function, in essence. And it's documented, and it's not the natural thing one would do (Integer(1.1) behaves as you would like).

comment:7 Changed 11 years ago by leif

  • Status changed from needs_review to positive_review

The Python behavior could be "catched" by the preparser. There have recently been long discussions about Sage's "coercion model"...


Applied Karl-Dieter's patch on 4.4.3.alpha0.

sage -t -long devel/sage/sage/rings passed all tests.

Positive review.

comment:8 Changed 11 years ago by leif

  • Keywords CDF conversion complex double added

make ptestlong also did not give errors related to this patch (again Sage 4.4.3.alpha0, Ubuntu 9.04 x86/32-bit).

comment:9 Changed 11 years ago by mhansen

  • Merged in set to sage-4.4.4.alpha0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.