API,MAINT: Make NpyIter_GetTransferFlags public and avoid old uses
#27998
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mattip
approved these changes
Dec 17, 2024
numpy/_core/src/multiarray/ctors.c
Outdated
| @@ -2795,7 +2786,8 @@ PyArray_CopyAsFlat(PyArrayObject *dst, PyArrayObject *src, NPY_ORDER order) | |||
| NpyIter_Deallocate(src_iter); | |||
| return -1; | |||
| } | |||
| needs_api |= (flags & NPY_METH_REQUIRES_PYAPI) != 0; | |||
| /* No need to worry about API use in unbuffered iterator */ | |||
| needs_api = (flags & NPY_METH_REQUIRES_PYAPI) != 0; | |||
There was a problem hiding this comment.
Should probably declare and set the value together.
Suggested change
| needs_api = (flags & NPY_METH_REQUIRES_PYAPI) != 0; | |
| int needs_api = (flags & NPY_METH_REQUIRES_PYAPI) != 0; |
This makes NpyIter_IterationNeedsAPI public, since there is no reason not to now that the flags are public.
…here Also remove any duplicate calls which are now much slower.
efd6688 to
71e33fd
Compare
|
Ping @ngoldbaum for awareness though, since it adds the public function. Also, will push one small change: The return value should be |
|
OK, put this in very happy to follow up if the docs should be improved (or anything else). |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This combines two related things:
NpyIter_GetTransferFlagsa public function. Now that we have these flags in the public API, there seems no reason not to make this public.NEEDS_APIflag every time (withREFS_OK) for use inNpyIter_IterationNeedsAPI. But we should generally not useNpyIter_IterationNeedsAPIanymore in NumPy. The reason is that we need to know what buffering needs, but already know anyway if we need the API for whatever we do with the dtypes. So:NpyIter_IterationNeedsAPIso that we avoid the unnecessary flag discovery (at least in throttled CPU mode ~5% of a trivial reduce).I completely removed one macro, because I didn't want the confusion of which version is used (or two macros).
This is ready for review, but marking as draft, because the reduce code move is probably only safe after gh-27883. (if we are lucky, the sanitizer CI run notices this)