Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork11
implement same_value casting for numpy <-> quadtype#161
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| template<typename T> | ||
| staticNPY_CASTING | ||
| staticint |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Hmm... I think this is OK? I didn't see any new warnings, but there are thousands of build warnings when using sleef and I might have missed it.
| .casting = NPY_SAME_VALUE_CASTING, | ||
| #else | ||
| .casting = NPY_UNSAFE_CASTING, | ||
| #endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I am not clear how this is used, doesn't the resolver actually state which casting is supported?
| #ifndef DISABLE_QUADBLAS | ||
| #include"../subprojects/qblas/include/quadblas/quadblas.hpp" | ||
| #include"quadblas/quadblas.hpp" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I needed this to build with-e, i.e.pip install -Csetup-args="-Dbuildtype=debug" -e . --no-build-isolation 2>&1 | tee /tmp/build.txt. Otherwise the build failed.
Ahh, this needs a nightly numpy build for CI. How is the best way to do that? |
Hmm, that's a good point that we need to wait for numpy releases go add features here when often we want to work on both in tandem. I'd be for adding a new CI job that depends on numpy I think ideally also you should be able to write this with conditional compilation so it works on older numpy versions, but maybe that's hard in practice. I don't think it's ahuge deal to raise the minimum requires numpy version here but also I think it's worth looking to see if we can avoid that. |
| *ret = from_quad<typename NpyType<T>::TYPE>(x, backend); | ||
| quad_value check = to_quad<typename NpyType<T>::TYPE>(*ret, backend); | ||
| if (backend == BACKEND_SLEEF) { | ||
| if (check.sleef_value == x.sleef_value) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can we just compare sleef values here? Should this use equality with NaN = NaN?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can we just compare sleef values here?
I was following the pattern from elsewhere. Is it guaranteed that the sleef backend is used?
Should this use equality with NaN = NaN?
Good point!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
In the SLEEF branch, I was thinking ofSleef_icmpeqq1(a, b) | (Sleef_iunordq1(a, a) & Sleef_iunordq1(b, b))
quaddtype/tests/test_quaddtype.py Outdated
| deftest_same_value_cast(): | ||
| # This will fail if compiled with NPY_TARGET_VERSION NPY<2_4_API_VERSION |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Could this text check the numpy version and be ignored for <2.4?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It needs both installednumpy.__version__ >=2.23 and that numpy_quadtype is built with the 2.24+ C-API
| #defineNPY_NO_DEPRECATED_APINPY_2_0_API_VERSION | ||
| #defineNPY_TARGET_VERSIONNPY_2_0_API_VERSION | ||
| #defineNPY_NO_DEPRECATED_APINPY_2_4_API_VERSION | ||
| #defineNPY_TARGET_VERSIONNPY_2_4_API_VERSION |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is a bit trickier unless you don't care about supporting NumPy <2.4. But if you don't, make sure that the target version is defined in (and matches up) with the file that does the actual NumPy import/loading. (Because that will check runtime compatibility.)
You can lie (I really don't mind), but you cannot use the flag at runtime unless you also checkPyArray_RUNTIME_VERSION at runtime and not at compile time.
As is, if you compile with NumPy 2.4 (which you should!), the casts will be broken on 2.3.
(could also just define the flag here manually. Wecould include thePyArray_RUNTIME_CHECK into the numpy headers as well if you prefer -- to basically simplify exactly this type of code that wants to use it if available. The compat headers have a lot of examples for such code.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
make sure that the target version is defined in (and matches up) with the file that does the actual NumPy import/loading
Good point, that isquaddtype_main.c.
but you cannot use the flag at runtime unless you also check PyArray_RUNTIME_VERSION at runtime
Did you meanPyArray_GetNDArrayCFeatureVersion() >= NPY_2_4_API_VERSION?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I meanPyArray_RUNTIME_VERSION >= NPY_2_4_API_VERSION.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
How does that work at runtime? It is not exported as a constant from_multiarray_umath.so
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It's injected as a constant into your library, next to the API table. Dunno if that was the right choice, but there we are :).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
PyArray_RUNTIME_VERSION isruntime! Not a pre-processor macro.
This comment was marked as off-topic.
This comment was marked as off-topic.
Boosting the comment about taking NAN into account when comparing float values which is still TBD. Also testing is not that extensive for edge cases.
|
Closes#153 to implement
same_valuecasting.I did not look at quad <-> quad casting. CC@seberg for visibility, as far as I can tell the
NPY_SAME_VALUE_CASTING_FLAGwas sufficient to percolate correctly through the can_cast mechanism.