BUG: Make floating remainder ufunc more exact. #7237
Conversation
|
You're getting failure on Travis due to c compiler warnings. I'm nervous that this is a lot of lines of code and subtle semantics to be putting into a b4 release :-/ Would it make more sense to merge this to master and revert 34c2369 on 1.11? |
|
No ;) It's not very subtle, actually. |
|
Search and replace error. |
|
I guess I'm just nervous because 34c2369 also seemed like a simple and obvious thing, but then it caused a bunch of downstream test failures and redesign during the beta period. So now we have a new simple and obvious change, and no beta period :-). I know that the downstream test failures are partly just them being silly about floating point tolerances, but as a point of information, have you checked whether this patch fixes the failures in astropy? |
|
(Also FYI you missed the |
6db4107 to
7b018b2
Compare
|
What bothers me is that the scalar tests didn't catch it, they should have. |
7b018b2 to
03ffddf
Compare
|
There is certainly going to be another beta, @mhvk owes us a masked array fix and we need to undo the integer indexing. |
|
Certainly fixes the problem reported. Have you read the tests? |
|
I didn't figure on merging this right away, I wanted to see the tests and I need to revisit after some rest. |
|
I haven't (yet) read through the logic of these tests or the astropy tests, beyond checking that the astropy failures involved a comparison that only failed by a few ulps -- I figured you were doing that ;-) |
|
Ha, the scalar slots for |
I added tests for exact results for small integers. Only |
The intent here is to make sure the following is true for floating numbers x (dividend) and y (divisor) and r = remainder(x, y). * If both x and y are small integer floats, r is exact. * The sign of r is the same as the sign of y, including signed zero. * The magnitude of r is strictly less than the magnitude of y. * y ~= r + y*floor(x/y), i.e., r is compatible with floor. Remainder functions are also added to npy_math for all supported floats 'efdg'. This helps keep scalar and array results in sync. Explicit loops are also made for remainder as the speedup over using generic loops is about 20%. Note that the NumPy version of remainder differs from that in Python, as the latter is based around the fmod function rather than floor. Closes numpy#7224.
Test that remainder produces exact results for small ints that are exactly represented by floats with small exact mantissas. This effective tests for the same numbers scaled by powers of two. Test corner cases. This tests cases involving zero division and infs that should result in nans as well as cases involving negative numbers very close to zero. Previous divmod tests located in test_multarray are moved into test_umath and renamed for remainder. They are a better there, as remainder is a ufunc.
03ffddf to
e99bf35
Compare
|
I'm tending towards a simple |
|
@charris - sounds good to me! |
|
I have been convinced that the best way to do this is to copy the Python implementation. Instead of trying to make the pair ( |
|
@charris - sounds good! |
|
OK, closing this, new PR in progress. |
The intent here is to make sure the following is true for floating
numbers x (dividend) and y (divisor) and r = remainder(x, y).
Remainder functions are also added to npy_math for all supported floats
'efdg'. This helps keep scalar and array results in sync. Explicit loops
are also made for remainder as the speedup over using generic loops is
about 20%.
Note that the NumPy version of remainder differs from that in Python, as
the latter is based around the fmod function rather than floor.
Closes #7224.