-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WIP: MAINT: avoid numpy internals in {sg}et_array_base #2528
Conversation
Any ideas why the c++ tests are failing but the c are passing? |
The tests fail with this error:
C isn't as strict as C++ here. |
Cython/Includes/numpy/__init__.pxd
Outdated
else: | ||
return <object>arr.base | ||
cdef inline object get_array_base(object arr): | ||
return <object>arr.base |
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.
No need to cast here, since this is clearly using Python object access, which returns an … object! :)
But why isn't this using PyArray_BASE()
? (Probably not expected to occur in performance critical 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.
I wanted it to go through the python-level attribute access, maybe that is extreme. I will redo this with PyArray_BASE.
Cython/Includes/numpy/__init__.pxd
Outdated
@@ -719,6 +719,7 @@ cdef extern from "numpy/arrayobject.h": | |||
object PyArray_CheckAxis (ndarray, int *, int) | |||
npy_intp PyArray_OverflowMultiplyList (npy_intp *, int) | |||
int PyArray_CompareString (char *, char *, size_t) | |||
int PyArray_SetBaseObject(object, object) |
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.
The first argument is an ndarray
.
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.
fixed
Cython/Includes/numpy/__init__.pxd
Outdated
@@ -973,22 +974,12 @@ cdef extern from "numpy/ufuncobject.h": | |||
|
|||
int _import_umath() except -1 | |||
|
|||
cdef inline void set_array_base(object arr, object base): | |||
Py_INCREF(base) | |||
PyArray_SetBaseObject(arr, base) |
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.
Just keep ndarray arr
in the function signature of set_array_base()
. That's what the signature was, and that's also what's needed for this call.
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.
ok
Thanks for the help. I admit not knowing how the failing tests are generated, so I couldn't run them before pushing. I also left some lingering questions around the use of |
Reading the documentation helps, now running tests locally |
1e638cf
to
21f1a73
Compare
@@ -395,7 +395,7 @@ cdef extern from "numpy/arrayobject.h": | |||
npy_intp PyArray_DIM(ndarray, size_t) | |||
npy_intp PyArray_STRIDE(ndarray, size_t) | |||
|
|||
# object PyArray_BASE(ndarray) wrong refcount semantics | |||
object PyArray_BASE(ndarray) #wrong refcount semantics? |
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.
The NumPy documentation isn't clear here, but I assume that PyArray_BASE()
is just a macro that returns a borrowed reference? If so, then the correct return type here is PyObject *
and not a normally refcounted object
. You'll then have to cast it to <object>
on use, which will turn it into an owned reference.
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.
yes, it's just a macro. Fixing to old logic
Cython/Includes/numpy/__init__.pxd
Outdated
return <object>arr.base | ||
|
||
base = PyArray_BASE(arr) | ||
# Do we need to convert NULL -> None? |
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 guess so, as it was done before.
Cython/Includes/numpy/__init__.pxd
Outdated
|
||
base = PyArray_BASE(arr) | ||
# Do we need to convert NULL -> None? | ||
# Do we need to incref base or is that done by cython? |
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.
See my comment regarding the macro signature.
It seems the clang failures are unrelated |
… numpy declarations. See #2528.
In the discussion of issue #2498, it seems cython uses direct access of ndarray structure base attribute in
set_array_base
andget_array_base
. This bypasses much of the NumPy error checks, and can lead to violating assumptions NumPy makes as to the correctness of the base attribute.This PR refactors the functions to use NumPy APIs. I am not sure it is correct, I could find no tests for the functionality, and truthfully struggle to see when these functions could be properly used. I would prefer to deprecate them.
NumPy uses the base attribute internally when
The simple code in these functions does none of the data attribute manipulations done by those two internal uses.