Repository navigation
ENH: Removed remnants of Python 2 buffer protocol from NumPy internals - #31176
Anushreebasics wants to merge 2 commits into
Conversation
SummaryIt makes three main changes:
@ngoldbaum @jorenham please review |
eaa9145 to
da1a38a
Compare
ngoldbaum
left a comment
ngoldbaum
left a comment
There was a problem hiding this comment.
Left some comments. FWIW it's OK if you used AI for PRs like this, you just need to disclose it.
| npy_intp len; | ||
| int flags; | ||
| } _PyArray_Chunk; | ||
| #else |
There was a problem hiding this comment.
Why add this indirection if you're not going to deprecate using the struct or change it? I'd just keep it around and use the public version internally.
There was a problem hiding this comment.
You could also figure out a way to signal to users that this struct is deprecated. In that case it makes sense to add the indirection.
| PyArray_BufferConverter(PyObject *obj, _PyArray_Chunk *buf) | ||
| { | ||
| if (DEPRECATE("PyArray_BufferConverter is deprecated and will be removed in a future version. Use the standard Python buffer protocol instead.") < 0) { | ||
| return NPY_FAIL; |
There was a problem hiding this comment.
there should be a test for this deprecation
There was a problem hiding this comment.
Sorry for not looking earlier, if you are up for an iteration.
It would be good to add a (Deprecated NumPy 2.5) as well as a /* Deprecated NumPy 2.5, 2026-04 */ style comment to this (breaking the long line would be nice also).
The deprecation test-case is nice. And I don't mind keeping it where it is, although it would be nice to cover the error path by making sure DeprecationWarning is raised an error.
(That is part of why we have a test_deprecated, moving the test there may be the nicest solution.)
Either way, all of these are indeed more nits and we could probably merge without as well.
da1a38a to
7fb72af
Compare
|
@ngoldbaum please review the changes |
ngoldbaum
left a comment
ngoldbaum
left a comment
There was a problem hiding this comment.
Looks good! I marked it for the 2.5 release. Before this can be merged it needs a release note though. See doc/release/upcoming_changes/README.rst for instructions.
|
@ngoldbaum I have added the release notes, please review the changes |
|
I think this is OK to merge. Maybe @seberg could take a look and hit the merge button though? |
|
@Anushreebasics sorry for dropping this - it looks like there's a merge conflict now. Would you mind fixing that? |
|
I think I commented on a resolved convo, so it got lost. But it would be good to test add Either way, both are smaller things if Nathan prefers to just go ahead that is fine with me, but if there is a conflict to resolve... |
fixes #30813
First time committer introduction
Hi NumPy team, I’m a first-time contributor. I use NumPy regularly for array-heavy Python work, mainly for data processing and numerical experimentation. While working with NumPy internals/C-API docs, I noticed issue #30813 around legacy Python 2 buffer-protocol language and wanted to help modernize and clarify that path. This contribution is my attempt to reduce confusion for future contributors and keep the API/docs aligned with current behavior.
AI Disclosure
no AI tools were used