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

Don't use "long double" functions for Pynac #20531

Closed
jdemeyer opened this issue May 2, 2016 · 17 comments
Closed

Don't use "long double" functions for Pynac #20531

jdemeyer opened this issue May 2, 2016 · 17 comments

Comments

@jdemeyer
Copy link

jdemeyer commented May 2, 2016

There seems to be no reason to use the long double functions like sage_logl for Pynac. It interacts only with Python objects and Python's float corresponds to double in C.

CC: @rwst @embray

Component: cython

Author: Jeroen Demeyer

Branch/Commit: 20cdb45

Reviewer: Erik Bray, Ralf Stephan

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

@jdemeyer jdemeyer added this to the sage-7.2 milestone May 2, 2016
@jdemeyer
Copy link
Author

jdemeyer commented May 2, 2016

@jdemeyer
Copy link
Author

jdemeyer commented May 2, 2016

New commits:

491093cReturn 0 instead of NULL to fix compiler warning
b5ff931Do not use "long double" functions for Pynac

@jdemeyer
Copy link
Author

jdemeyer commented May 2, 2016

Commit: b5ff931

@embray
Copy link
Contributor

embray commented May 2, 2016

comment:3

I don't know anything about Pynac but LGTM otherwise.

@rwst
Copy link

rwst commented May 3, 2016

comment:4

LGTM too. I have put your real name in the Reviewer: field and set positive for you.

@rwst
Copy link

rwst commented May 3, 2016

Reviewer: Erik Bray, Ralf Stephan

@kiwifb
Copy link
Member

kiwifb commented May 3, 2016

comment:5

Actually one the commit will fix a QA problem reported by portage in sage-on-gentoo. Quite happy about that.

@vbraun
Copy link
Member

vbraun commented May 4, 2016

@vbraun
Copy link
Member

vbraun commented May 5, 2016

Changed commit from b5ff931 to none

@vbraun
Copy link
Member

vbraun commented May 5, 2016

comment:7

On OSX:

sage -t --long src/sage/symbolic/pynac.pyx
**********************************************************************
File "src/sage/symbolic/pynac.pyx", line 1795, in sage.symbolic.pynac.py_lgamma
Failed example:
    py_lgamma(4.r)
Expected:
    1.791759469228055
Got:
    1.7917594692280552
**********************************************************************
File "src/sage/symbolic/pynac.pyx", line 1797, in sage.symbolic.pynac.py_lgamma
Failed example:
    py_lgamma(4r)
Expected:
    1.791759469228055
Got:
    1.7917594692280552
**********************************************************************
1 item had failures:
   2 of   7 in sage.symbolic.pynac.py_lgamma

@vbraun vbraun reopened this May 5, 2016
@jdemeyer
Copy link
Author

jdemeyer commented May 5, 2016

comment:8

Interesting. So on OS X, (double)lgammal(4.0) is not the same as lgamma(4.0)

@jdemeyer
Copy link
Author

jdemeyer commented May 5, 2016

comment:9

...which isn't that surprising perhaps, since lgamma() probably uses a less precise algorithm than lgammal(). Anyway, the error is still less than 1 ulp, so I will just add some # abs tol.

@jdemeyer
Copy link
Author

jdemeyer commented May 5, 2016

Changed branch from b5ff931 to u/jdemeyer/b5ff9318c0bfa9720f6947f2fcdceb8f7fe2279d

@jdemeyer
Copy link
Author

jdemeyer commented May 5, 2016

Commit: 20cdb45

@jdemeyer
Copy link
Author

jdemeyer commented May 5, 2016

New commits:

491093cReturn 0 instead of NULL to fix compiler warning
b5ff931Do not use "long double" functions for Pynac
20cdb45Fix py_lgamma() doctests for OS X libm

@kiwifb
Copy link
Member

kiwifb commented May 5, 2016

comment:12

Trivially putting it back to positive review.

@vbraun
Copy link
Member

vbraun commented May 18, 2016

Changed branch from u/jdemeyer/b5ff9318c0bfa9720f6947f2fcdceb8f7fe2279d to 20cdb45

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