FEAT: Implementing same_value casting rule in quaddtype
#246
Conversation
New things fixed and added
|
| return Sleef_negq1(QUAD_ZERO); // -0.0 | ||
| if (Sleef_icmpeqq1(result, QUAD_PRECISION_ZERO)) { | ||
| if (Sleef_icmpltq1(*b, QUAD_PRECISION_ZERO)) { | ||
| return Sleef_negq1(QUAD_PRECISION_ZERO); // -0.0 |
There was a problem hiding this comment.
we have a const for negative zero
There was a problem hiding this comment.
There's a subtle issues in using that, we didn't used QUAD_PRECISION_NEG_ZERO anywhere because qutil gave the logical representation but the sign information is stored in the actual IEEE 754 binary format, not in the integer literal itself. Basically the sign is encoded in the high mantissa bits. Simply putting a minus sign in front of 0x0 doesn't create a negative zero representation.
I can replace it with a valid representation maybe in future PRs, for now that is just an artifact in the code.
juntyr
left a comment
juntyr
left a comment
There was a problem hiding this comment.
LGTM with minor nits addressed
This is still valid, outside this are just tweaks and refactoring to make things work in stable manner |
|
I can take a look after Jan 11. |
|
@SwayamInSync do you mind waiting a couple weeks on this? I'd like to have Matti look because he added In the meantime: one thing that would help is to make the PR more atomic. There's a fair amount of renaming and other stuff like that you could do in another PR then rebase this once the other PR is merged to reduce the diff size. In general I'd much rather review several smaller more focused PRs rather than one big PR. |
|
I was hoping for a faster timeline to the release (since numpy-quaddtype is used in my research) but life is life and I'll adapt my own timelines if need be |
Right, I also didn't want to make it this big, but it the design choice of using union with SIMD types in C++ gave a lot of ABI breaks (as |
|
Cool so the diff got down now, mostly again covered by |
|
Hi @mattip gentle ping to get this in your radar. |
|
I'm also hoping that we can merge this PR and publish v1.0 soon |
ngoldbaum
left a comment
ngoldbaum
left a comment
There was a problem hiding this comment.
I did a pass over the C code and spotted some issues. I'd appreciate it if you could look elsewhere for similar patterns to the main issue I spotted: setting an error in an error path when an error is already set
| } | ||
|
|
||
| // Helper function for quad-to-quad same_value check (inter-backend) | ||
| // NOTE: the inter-backend uses `double` as intermediate, |
There was a problem hiding this comment.
Does SLEEF not have any utilities for working with 80 bit floats? It’d be nice to not have this restriction. But also not critical.
|
|
||
| // Perform same_value check if requested | ||
| if (same_value_casting) { | ||
| if (quad_to_string_same_value_check(&in_val, str_buf, str_size, backend) < 0) { |
There was a problem hiding this comment.
Is it possible for the same value not to be preserved here? I don't think it is - StringDType shouldn't truncate data. That means this check isn't necessary.
| } | ||
|
|
||
| static inline int | ||
| quad_to_string_same_value_check(const quad_value *in_val, const char *str_buf, npy_intp str_len, |
There was a problem hiding this comment.
Maybe you could avoid allocating a temporary buffer to store a string if you knew how many digits are necessary to represent the quad precisely. Is that exposed by SLEEF?
I ask because this whole process - allocating and freeing a temporary and parsing the truncated string, seems very slow and probably adds a lot of overhead to the cast operation for the fixed-width strings.
| val_str); | ||
| } | ||
| else { | ||
| PyErr_SetString(PyExc_ValueError, |
There was a problem hiding this comment.
This is wrong: you shouldn't set a new error here if quad_to_string_adaptive_cstr already set an error. I'd just bubble up the error instead of trying to give more context here.
| val_str); | ||
| } | ||
| else { | ||
| PyErr_SetString(PyExc_ValueError, |
There was a problem hiding this comment.
same issue here - an error is already set, setting a new one without resetting the error indicator is wrong.
| val_str); | ||
| } | ||
| else { | ||
| PyErr_SetString(PyExc_ValueError, |
4 participants
closes #153
As per the title