Repository navigation
BUG: Correct sin/cos float64 range check functions - #23695
Conversation
|
Could you tighten the tests to proof that this fixes things? |
When I translated range checks for [sin](https://github.com/ARM-software/optimized-routines/blob/91d5bbc3091fa568e6856c7c41f9d7492d5957df/math/v_sin.c#L68): ```c cmp = v_cond_u64 ((ir >> 52) - TinyBound >= Thresh); ``` and [cos](https://github.com/ARM-software/optimized-routines/blob/91d5bbc3091fa568e6856c7c41f9d7492d5957df/math/v_cos.c#L56): ```c cmp = v_cond_u64 (v_as_u64_f64 (r) >= v_as_u64_f64 (RangeVal)); ``` They ended up the wrong way around, this corrects it.
Co-authored-by: Pierre Blanchard <[email protected]>
05ac576 to
9bbd76a
Compare
|
@seberg, apologies, minor panic seeing the incorrect functionality, have added a test now 😸 |
| dtype=np.float64 | ||
| ) | ||
| np.sin(underflow_trigger) | ||
| np.cos(underflow_trigger) |
There was a problem hiding this comment.
OK, I guess before sin always ran into it at least.
Seems mac gets the underflows wrong here. Honestly, I doubt we should bother much about underflow warnings, but its nicer to get this right for now. I suspect we are skipping/xfailing mac often enough that we can just steal that here.
Just to be sure: This is really just about the warnings and not about the values?
There was a problem hiding this comment.
Yip,np.sin would throw an exception here due to fenv but would otherwise be fine, the same is true for pure optimised routines:
_ZGVnN2v_sin(0x1.f37f47a03f82ap-511) is 0x1.f37f47a03f82ap-511 +0 ulp, got except 0x18 wrongly set: 0x8
I added np.cos to prove we didn't instead introduce a FloatingPointException by removing the bounding.
|
Thanks, lets hope the discussion on the original PR doesn't mean more follow-ups are necessary. |
When I translated range checks for sin:
and cos:
They ended up the wrong way around, this corrects it.