FEAT: Implementing same_value casting rule in quaddtype
#246
Conversation
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.
There was a problem hiding this comment.
They does not have any explicit cross-platform utilities for 80-bit floats.
That's why only values perfectly castable from "longdouble" -> "quad" are those that are under the "double" range (because of this intermediate)
If someone still wants the perfect cast then ld -> string -> quad is the route
|
|
||
| // 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.
There was a problem hiding this comment.
Oh yes right, my bad
we shouldn't be needing same_value check here.
| } | ||
|
|
||
| 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.
There was a problem hiding this comment.
There's a nuance here, let me explain so 36 for exact decimal digits and 42 for scientific notation
They are not exposed by SLEEF, we keep the max string len to 50 (42 + some buffer)
Now when casting a QuadPrecision value to a fixed-width string (like U10) then the dragon4 will generate full buffer representation but it will get truncated to only 10 chars. Then here we in same_value check we need to re-create the null-terminated buffer.
Oh good point, that buffer can be in local stack, we don't be needing the heap allocation here
There was a problem hiding this comment.
- 1 character for sign
- 1 character for leading digit
- 1 character for decimal point
- 36 significant digits
- 1 character for e
- 1 character for exponent sign
- 4 characters for exponent (max is 4932)
- 1 null terminator
So it should be total of 46 characters (still 50 is safe)
| 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, |
mattip
left a comment
mattip
left a comment
There was a problem hiding this comment.
LGTM, pending the already-mentioned corrections. Tests should be catching edge cases.
|
@ngoldbaum these new changes should address all the reviews, I found 2 more cases of error-overriding and patched them here (inside |
ngoldbaum
left a comment
ngoldbaum
left a comment
There was a problem hiding this comment.
Thanks for your patience here!
|
Great! Merging this in. |
|
Thanks! How long do you think will it now take to release v1.0? |
4 participants
closes numpy/numpy-quaddtype#26
As per the title