Refactor: General refactors + quad2quad inter-backend fixes #247
Conversation
|
We could've also refactored the |
| { | ||
| long double ld = in_val.longdouble_value; | ||
| if (std::isnan(ld)) { | ||
| out_val.sleef_value = (!ld_signbit(&ld)) ? QUAD_PRECISION_NAN : QUAD_PRECISION_NEG_NAN; |
There was a problem hiding this comment.
you can get rid of the double negation here and switch the arms
| // fix can be catching the cases and returning corresponding type value without casting | ||
| if (backend == BACKEND_SLEEF) { | ||
| return (npy_byte)Sleef_cast_to_int64q1(x.sleef_value); | ||
| return (npy_byte)Sleef_cast_to_int64q1(x->sleef_value); |
There was a problem hiding this comment.
can we use static_cast's in the from_quad methods as well?
There was a problem hiding this comment.
Sure, here we can, although the casting coming from a int64 to lower variants so the current code is also safe
juntyr
left a comment
juntyr
left a comment
There was a problem hiding this comment.
I left some nits but functionality-wise this looks good to go to me. Thanks @SwayamInSync!
ngoldbaum
left a comment
ngoldbaum
left a comment
There was a problem hiding this comment.
Looks good, just one small question inline.
| if (pos < maxPrintLen - 1) { | ||
| buffer[pos++] = '-'; | ||
| } | ||
| } |
There was a problem hiding this comment.
Should we also make this change in NumPy?
There was a problem hiding this comment.
Maybe for the sake of completeness yeah, I am not sure of the actual usecase of -nan
An even important case is -0.0 as I've seen some people use it as signal and it is working correct in numpy
There was a problem hiding this comment.
-0.0 is, unfortunately, quite important, so that you can go from e.g. -1/inf=-0 back to -1/-0=inf.
-NaN is more niche but worth preserving nonetheless
|
Thanks, everyone |
3 participants
This PR contains the unrelated refactors happend inside #246 (the commits were inter-twined with
same_valuefeat so had to pick them manually here, can cause conflict in that PR but should be resolvable)quad_valueunion