MAINT: Translate ieee754.c.src to C++ using templates. #21367
Conversation
serge-sans-paille
force-pushed
the
feature/road-to-cxx-ieee754
branch
from
de349a3 to
7085883
Compare
charris
changed the title
charris
added
the
C++
serge-sans-paille
force-pushed
the
feature/road-to-cxx-ieee754
branch
7 times, most recently
from
e222bed to
00766c3
Compare
|
@charris should be good for review now! |
numpy/core/src/npymath/ieee754.cpp
Outdated
| npy_uint64 lx; | ||
| npy_longdouble u; | ||
| constexpr npy_longdouble eps = 2.465190328815662e-32; // 0x1.0000000000000p-105L |
There was a problem hiding this comment.
| constexpr npy_longdouble eps = 2.465190328815662e-32; // 0x1.0000000000000p-105L | |
| constexpr npy_longdouble eps = 2.465190328815662e-32L; // 0x1.0000000000000p-105L |
Seems like this requires the L, or the value gets cast to double (and is probably slightly off).
There was a problem hiding this comment.
I wonder if a test for his makes sense? But seems fine to skip. I don't think this is normally used.
I am not sure I trust this. This is double-double only, but that should have a lot more digits I think? Maybe we can calculate it as a power of two instead?
There was a problem hiding this comment.
I checked and the internal representation of the two values is the same as I commited them.
There was a problem hiding this comment.
But to avoid confusion, I updated the patch with the 'L'-suffixed value :-)
There was a problem hiding this comment.
How did you validate it?
In [1]: np.longdouble("2.465190328815662e-32") - 2.465190328815662e-32
Out[1]: 1.07979694287724704104e-48
And this is 80bit extended precision longdouble. IBM double-double has a much bigger mantissa. Probably this function isn't even needed, but I am not 100% sure (IIRC aarch64 or so may not provide nextafter even though it is C99).
Maybe use exp2l to calculate it? That is how we do it for np.finfo as well (in Python).
There was a problem hiding this comment.
(oopsie, code just updated) https://godbolt.org/z/EbaqdfPsP
There was a problem hiding this comment.
I am worried that this is aarch64 (or similar): https://godbolt.org/z/53f8j4jY7
There was a problem hiding this comment.
ohhh, I got it the wrong way. It actually must be without the L, because than it is the correct double precision value (i.e. exact in binary). But with the L it is not an exact in binary.
There was a problem hiding this comment.
indeed. Let's do a math call then.
I only did that where it mattered for overload resolution. |
serge-sans-paille
force-pushed
the
feature/road-to-cxx-ieee754
branch
3 times, most recently
from
240dbac to
4e353a1
Compare
|
The the failure of the simd test is new and unrelated. Something upstream has changed (again). |
Replace them by regular C++ templates.
Clang-format it, and update build system accordingly.
… [-Wparentheses]'
- hex float literal are not supported in C++ - avoid overload issues when numpy_longdouble ultimately is equal to double - _statusfp2 exepcts unsigned arguments
serge-sans-paille
force-pushed
the
feature/road-to-cxx-ieee754
branch
from
4e353a1 to
c99d908
Compare
ok! I restarted the test just in case it automagically got fixed :-) |
|
close/reopen |
|
If Chuck is happy, I think we can go ahead and clean up anything that comes up later. |
|
OK, let me just put this in, we can always fix it up and it seemed like a pretty straight forward conversion. |
No description provided.