BUG: Overflow in tan and tanh for large complex values #5510
Conversation
|
Squashed commits of #3010 and rebased on master. @juliantaylor Might want to rebase this for backport pending review. |
|
One aspect of this that was never really resolved were the build system changes. This compiles and executes some tests to decide whether to use our internal function or the libc version. Which I guess is fine but never really seemed ideal. |
| @@ -33,7 +33,7 @@ class TypeDescription(object): | |||
| ---------- | |||
| type : str | |||
| Character representing the nominal type. | |||
| func_data : str or None or FullTypeDescr, optional | |||
| func_data : str or None or FullTypeDescr or FuncNameSuffix, optional | |||
There was a problem hiding this comment.
Hmm, might want to remove this change.
There was a problem hiding this comment.
FuncNameSuffix is used in master, so keep this.
|
Needs fixups.
|
| @@ -370,6 +371,11 @@ PyMODINIT_FUNC initumath(void) | |||
| PyDict_SetItemString(d, "euler_gamma", s = PyFloat_FromDouble(NPY_EULER)); | |||
| Py_DECREF(s); | |||
|
|
|||
| #if defined(NPY_PY3K) | |||
| s = PyDict_GetItemString(d, "true_divide"); | |||
There was a problem hiding this comment.
Things were working before this, why make this change?
There was a problem hiding this comment.
These lines have just been moved. This whole #if was added in cfc5c3b.
np.tanh(1000+0j) gives nan+nan*j instead of 1.0+0j. np.tan(0+1000j) gives nan+nan*j instead of 1j. I've imported the implementation for ctanh from FreeBSD's math library which handles large arguments correctly and fixes this bug and the equivalent bug in np.tan. The problem here is that importing the function into npy_math_complex.c and adding a config check causes us to use the implementation from glibc on Linux, which also has this bug. Although it seems to have been fixed in glibc last April (http://sourceware.org/bugzilla/show_bug.cgi?id=11521). I see several things that could be done here, the easiest is probably to use our version of ctanh unconditionally. Although there are a multitude of ways to go about that, for instance should I remove the implementation from npy_math_complex.c and just place it directly in umath/funcs.inc, where the buggy version was? Or should I just remove the config check and add a note about it somewhere? Closes numpy#2321.
Omitted numpy/core/src/npymath/test_c99complex.c, which is a jungle of macro magic.
charris
force-pushed
the
cleanup-gh-3010
branch
from
47eb151
to
ff5a1c2
Compare
| @@ -209,6 +209,32 @@ def check_prec(prec): | |||
| else: | |||
| priv.extend([(fname2def(f), 1) for f in flist]) | |||
|
|
|||
| flist = [f + prec for f in C99_COMPLEX_FUNCS_CHECKED] | |||
There was a problem hiding this comment.
Makes me wonder if these tests shouldn't be done in python.
There was a problem hiding this comment.
There was some discussion of using ctypes to call these functions from python. I didn't go down that route since we then need to find libc ourselves, but doing it this way allowed the tool chain to find it for us. I suppose we could make a tiny module that simply exposes them to python and then use that for the tests.
| @@ -258,8 +261,11 @@ float npy_copysignf(float x, float y); | |||
| float npy_nextafterf(float x, float y); | |||
| float npy_spacingf(float x); | |||
|
|
|||
| float npy_ldexpf(float x, int exp); | |||
There was a problem hiding this comment.
npy_ldexp and npy_frexp in all three flavors already exist in master. These came in as part of #4852, which pulled the ldexp and frexp changes from this.
|
@ewmoore I have a simplified version of this PR. Given the problems with casinh in gcc < 4.8, I'm inclined to simply restrict detection of system libraries to the current set and use your new routines for the rest. Other sources of these library routines that we could use in the future come with the languages D (Boost license) and GO (three clause BSD), both of which would need translation to C. The D library seems to have better support of quad precision (SUN). It is difficult to understand how gcc could have gotten things wrong the way they did. We could also simply blacklist some compilers, but the maintenance burden of that is likely too heavy. |
Rebase of #3010.
np.tanh(1000+0j)givesnan+nan*jinstead of1.0+0j.np.tan(0+1000j)givesnan+nan*jinstead of1j.I've imported the implementation for ctanh from FreeBSD's math library
which handles large arguments correctly and fixes this bug and the
equivalent bug in np.tan.
The problem here is that importing the function into npy_math_complex.c
and adding a config check causes us to use the implementation from glibc
on Linux, which also has this bug. Although it seems to have been fixed
in glibc last April
(http://sourceware.org/bugzilla/show_bug.cgi?id=11521).
I see several things that could be done here, the easiest is probably to
use our version of ctanh unconditionally. Although there are a multitude
of ways to go about that, for instance should I remove the
implementation from npy_math_complex.c and just place it directly in
umath/funcs.inc, where the buggy version was? Or should I just remove
the config check and add a note about it somewhere?
Closes #2321.