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

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

Closed
jasongrout opened this issue May 4, 2010 · 13 comments
Closed

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

jasongrout opened this issue May 4, 2010 · 13 comments

Comments

@jasongrout
Copy link
Member

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 #5400 comment:12 and on sage-devel), we should make float conversion succeed if the imaginary part is zero.

Component: basic arithmetic

Keywords: CDF conversion, complex double

Author: Jason Grout

Reviewer: Karl-Dieter Crisman, Leif Leonhardy

Merged: sage-4.4.4.alpha0

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

@jasongrout
Copy link
Member Author

Attachment: complex-float.patch.gz

@jasongrout
Copy link
Member Author

comment:2

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

@nexttime
Copy link
Mannequin

nexttime mannequin commented May 6, 2010

comment:3

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()?

@kcrisman
Copy link
Member

Author: Jason Grout

@kcrisman
Copy link
Member

comment:4

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

@kcrisman
Copy link
Member

Reviewer: Karl-Dieter Crisman, Leif Leonhardy

@kcrisman
Copy link
Member

Attachment: trac_8869.patch.gz

Based on 4.4.2, apply only this patch

@nexttime
Copy link
Mannequin

nexttime mannequin commented May 26, 2010

comment:5

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... ;-)

@kcrisman
Copy link
Member

comment:6

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

@nexttime
Copy link
Mannequin

nexttime mannequin commented May 27, 2010

comment:7

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.

@nexttime
Copy link
Mannequin

nexttime mannequin commented May 27, 2010

comment:8

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

@nexttime
Copy link
Mannequin

nexttime mannequin commented May 27, 2010

Changed keywords from none to CDF conversion, complex double

@mwhansen
Copy link
Contributor

mwhansen commented Jun 6, 2010

Merged: sage-4.4.4.alpha0

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

4 participants