Conversation
|
Not a full review, will try to give this a read tomorrow.
Most likely a left over from the times before
Good question, not sure. I think I'd also prefer having the |
Icxolu
left a comment
There was a problem hiding this comment.
Thank you ❤️ I think this is a great start already. I left a few comments, mostly documentation related. We should also make sure to add a changelog entry for this.
| @@ -94,8 +146,8 @@ impl PyArrayAPI { | |||
|
|
|||
| impl PyArrayAPI { | |||
There was a problem hiding this comment.
Is there some page that we can link to where this mapping is documented for future reference?
| impl_api![0; PyArray_GetNDArrayCVersion() -> c_uint]; | ||
| impl_api![40; PyArray_SetNumericOps(dict: *mut PyObject) -> c_int]; | ||
| impl_api![41; PyArray_GetNumericOps() -> *mut PyObject]; | ||
| // Unused slot 40, was `PyArray_SetNumericOps` |
There was a problem hiding this comment.
Maybe we can add a comment here that slots 1-39 are type objects
|
|
||
| /// See [PY_ARRAY_API] for more. | ||
| pub struct PyArrayAPI(PyOnceLock<*const *const c_void>); | ||
| pub struct PyArrayAPI(PyOnceLock<NonNull<*const c_void>>); |
There was a problem hiding this comment.
Very nice 👍 Always great to have invariant encoded in the type system.
| } | ||
|
|
||
| /// Returns whether the runtime `numpy` version is 2.0 or greater. | ||
| pub fn is_numpy_2<'py>(py: Python<'py>) -> bool { |
There was a problem hiding this comment.
I think we need to look at the usages of this. These are used to conditionally cast structures, which we should not do anymore now that we are targeting a single ABI, right?
| @@ -1,2 +1,2 @@ | |||
| //! Low-Level bindings for NumPy C API. | |||
| //! | |||
There was a problem hiding this comment.
Maybe we can add a comment here that we are targeting the numpy 2 abi, and that is is abi compatible with numpy 1 for the api subset that numpy v2 exposes.
| } | ||
|
|
||
| }; | ||
| impl_array_type! { |
There was a problem hiding this comment.
Smart, I like the syntax and that this can now support returning type objects from different APIs.
This PR contains the implementation discussed in #534. The
npyffimodule has been updated to match the NumPy implementation of ABI v2 when theNPY_FEATURE_VERSIONmacro is set to 0x0000000c (v1.15). Some new macros are required for the implementation, I prefer to follow the same layout of NumPy to make easier future changes.The major changes of this PR are:
NonNullmarker is used to store the pointer contained in capsules.T([u8; 0]).npy_longdoublehave been commented; previously they were related tof64, but on some platforms long double is 80-bit extended precision.During the implementation, some doubts arose:
std::os::rawused? Usuallystd::ffiis preferred.Option<...>? I found that the usage ofOption<...>is more common in member definitions, probably even inpyo3. For example: