ENH: Add the Array Iterator API to Cython by zoj613 · Pull Request #24691 · numpy/numpy

zoj613 · 2023-09-13T06:16:59Z

This is a followup to #24658, and adds the Array Iterator API to the Cython 3.X init file as suggested by @seberg in #24658 (comment).

zoj613 · 2023-09-13T13:24:23Z

@seberg Any idea how I can resolve the error: error: invalid application of 'sizeof' to an incomplete type 'NpyIter' (aka 'struct NpyIter_InternalOnly') ? I tried declaring the npyIter struct but that doesn't help.

EDIT: Had to make the NpyIter_InternalOnly struct visible to the compilation unit by moving the implementation to a different file. It resolved the error.

…ternalOnly

seberg · 2023-09-18T09:34:29Z

Had to make the NpyIter_InternalOnly struct visible to the compilation unit by moving the implementation to a different file. It resolved the error.

You must not do that, we need that struct to remain private. You cannot wrap NpyIter in a normal cdef class, maybe there is a cdef struct, I would have to dig into cython a bit more. NpyIter is not a Python object and you can only wrap those.

seberg · 2023-09-18T09:36:19Z

I guess there may be a way to write it in this way, but maybe a good start wouold also be to not try to implement an nditer object (also note that np.nditer in Python is a wrapper around the NpyIter and a different object).

zoj613 · 2023-09-18T15:00:16Z

I guess there may be a way to write it in this way, but maybe a good start wouold also be to not try to implement an nditer object (also note that np.nditer in Python is a wrapper around the NpyIter and a different object).

Ah I see, I will make the change and just specify the signatures instead of implementing the nditer object.

zoj613 · 2023-09-18T17:48:20Z

@seberg your suggestion was implemented in 86408c5 (#24691)

seberg · 2023-09-19T10:01:24Z

+    int NpyIter_Deallocate(NpyIter* it)
+    int NpyIter_Reset(NpyIter* it, char** errmsg) nogil
+    int NpyIter_ResetToIterIndexRange(NpyIter* it, npy_intp istart,
+                                      npy_intp iend, char** errmsg) nogil


Thanks, just a quick comment, since you are iterating.

I would be tempted to not add nogil as itis only correct when errmsg is passed, and I frankly find it a bit awkward.
(I am not sure it is actually important API for an unused threading usecase, or more of a try to allow use without GIL but without a concrete use-case)

My main question is how much you checked the correct except ... statements. Pretty use NpyIter_Dealloc needs an except 0, so I think there may be a few more. Also clearly creating a new one needs except NULL. (in other words, we need to carefully review those)

Just addressed the above comment in e5c2883 (#24691) by removing the nogil and replacing it with except? NPY_FAIL. I also checked the implementation of of the functions listed and added appropriate except ... where needed. Please give another look when you can.

zoj613 · 2023-10-15T17:59:24Z

@seberg , I've been meaning to ask how would one be able to retrieve the numerical values of operands during iteration through the iternext() and dataptrarray pattern as shown here ? It seems that dataptrarray elements are char* type. Is there a numpy way of making use of these values to do numerical calculations using operand values at the current iteration index? I tried NpyIter_GetIterView but it just gives the full view of the numpy array of a particular operand which isn't very useful when using iternext to iterate over the broadcasted operands.

seberg · 2023-10-16T08:28:36Z

It seems that dataptrarray elements are char* type. Is there a numpy way of making use of these values to do numerical calculations using operand values at the current iteration index?

I don't understand. You need to cast them to the correct type to perform the operation of course (but the stride char sized, so that is correct to advance it). So double val = (<double *>dataptr[0])[0] in Cython, I guess.

zoj613 · 2023-10-16T15:57:18Z

It seems that dataptrarray elements are char* type. Is there a numpy way of making use of these values to do numerical calculations using operand values at the current iteration index?

I don't understand. You need to cast them to the correct type to perform the operation of course (but the stride char sized, so that is correct to advance it). So double val = (<double *>dataptr[0])[0] in Cython, I guess.

Ah, I see the issue I had was that I did not use the parenthesis around the cast pointer, which led to unexpected behaviour. Thanks. BTW, I was able to prototype the use of this API here

seberg

Thanks, a few comments. I think all of those ? are unnecessary. The test could use a tiny bit of docs maybe.

The only real comment is that you are not cleaning up thing in the test. And note that this is very much tedious, because it will often mean that you need a:

try:
    ....
finally:
    NpyIter_Dealloc(it)

in cython!

Maybe we should note that somewhere?

seberg · 2023-10-16T16:32:34Z

+    if it.has_delayed_bufalloc:
+        flags |= cnp.NPY_ITER_BUFFERED | cnp.NPY_ITER_DELAY_BUFALLOC
+    if it.has_multi_index:
+        flags |= cnp.NPY_ITER_MULTI_INDEX


Is extracting this from the Python version worth it?

In either case, it's fine but maybe add short doc string that says that this is a limited implementation that recreates the NpyIter wrapped by a Python created np.nditer for testing?

(I started reading it as if it was some generic function, but it really is just a quick testing mock-up, which is fair!)

it makes it easy to test the methods from cython if I extract what's already in the nditer object. It doesn't cover everything but allows some testing to be done easily. I added a docstring in 365b6ff

seberg · 2023-10-16T16:33:41Z

+
+def get_npyiter_size(it: "nditer"):
+    cdef cnp.NpyIter* cit = npyiter_from_npyiter_obj(it)
+    return cnp.NpyIter_GetIterSize(cit)


All of these are missing the NpyIter_Dealloc(cit) call! (Yes, it is unfortunate that we cannot avoid that.)

Added in 365b6ff

seberg · 2023-10-16T16:39:56Z

+    # functions for iterating an NpyIter object
+    ctypedef int (*NpyIter_IterNextFunc)(NpyIter* it)
+    ctypedef void (*NpyIter_GetMultiIndexFunc)(NpyIter* it, npy_intp* outcoords)
+    NpyIter_IterNextFunc* NpyIter_GetIterNext(NpyIter* it, char** errmsg) except? NULL


I don't think you need the ?. Pretty sure NULL is always an error? (same below)

This function has an annoying little feature that if you pass errmsg, it is actually nogil. Maybe that is the confusion? I think we just have to ignore that fact.

(There is a thought to that, that you can copy an iterator and run things in a multi-threaded environment. I am not aware that anyone has ever really done this and even if, I don't think this is the right way to design such an API.)

Added suggestion in 365b6ff#diff-1e081d27e4d07ed1eaa8fe4af755d81de8c1dc062c919559b9678788dfc691f3

seberg · 2023-10-16T16:45:15Z

+    npy_intp NpyIter_GetIterIndex(NpyIter* it) nogil
+    void NpyIter_GetIterIndexRange(NpyIter* it, npy_intp* istart,
+                                   npy_intp* iend) nogil
+    int NpyIter_GotoIterIndex(NpyIter* it, npy_intp iterindex) except? NPY_FAIL


No need for the ? here also. (Actually, same above, I don't think there is any function that needs the ?)

Is it really? I looked through the function implementations and it appears there are code paths that seemingly return the "error" values but with no exception set. For example, this code path for NpyIter_AdvancedNew seems to return the error value NULL but does not set an exception:

numpy/numpy/_core/src/multiarray/nditer_constr.c

Lines 355 to 361 in 79ccbd1

dtype = npyiter_get_common_dtype(nop, op,

op_itflags, op_dtype,

op_request_dtypes,

only_inputs);

if (dtype == NULL) {

NpyIter_Deallocate(iter);

return NULL;

. Or am I misinterpreting this piece of code? The same applies to NpyIter_Deallocate and several others I marked with except?

dtype == NULL means something set an error already. the point is if an error is set we indicate that, ``NPY_FAIL` is never returned unless an error is set. That is not typical.

Ah I see, I removed the ? in the except in 365b6ff#diff-1e081d27e4d07ed1eaa8fe4af755d81de8c1dc062c919559b9678788dfc691f3

zoj613 · 2023-11-11T03:25:49Z

@seberg I added the suggested changes. Not all of the cython functions can be easily tested in python since nditer and NpyIter are not fully compatible. Let me know if you have further suggestions regarding tests that could be added.

seberg · 2023-11-16T09:38:50Z

Thanks, I think those were the last changes. The try/except dance is a bit annoying in Cython, but can't be helped, I think.

Since numpy v2.0.0, the Iterator API is part of cython declarions. See the PR adding this feature: numpy/numpy#24691 . This means we do not need to manually import these declarations.

github-actions Bot added the 01 - Enhancement label Sep 13, 2023

zoj613 force-pushed the add_iterator_api branch 2 times, most recently from 5456312 to 47b2dd5 Compare September 13, 2023 06:33

ENH: Add the Array Iterator API to Cython

5e80aac

zoj613 force-pushed the add_iterator_api branch from 47b2dd5 to 5e80aac Compare September 13, 2023 16:40

zoj613 added 2 commits September 13, 2023 20:18

make NpyIter -> np.nditer work by moving the definition of NpyIter_In…

3bb5e96

…ternalOnly

Add some initial tests for attribute access

052aab7

zoj613 force-pushed the add_iterator_api branch from 5d0328d to 052aab7 Compare September 15, 2023 04:53

zoj613 added 2 commits September 15, 2023 17:48

Add test cases for nditer attribute access.

58ad093

dont create op_dtype arg to MultiNew

cbdb803

add more tests

41b443c

do not implement nditer at the cython level

86408c5

zoj613 added 2 commits September 18, 2023 18:18

rename some functions

6e86198

lint checks.pyx

4ec4356

seberg reviewed Sep 19, 2023

View reviewed changes

update func signitures

e5c2883

zoj613 marked this pull request as ready for review October 15, 2023 17:59

zoj613 requested a review from seberg October 15, 2023 17:59

seberg reviewed Oct 16, 2023

View reviewed changes

zoj613 added 3 commits November 10, 2023 14:25

address review comments by @seberg

b3430c8

Merge branch 'main' into add_iterator_api

3c4e2f8

remove ? from except

365b6ff

seberg merged commit bdb0c13 into numpy:main Nov 16, 2023

zoj613 mentioned this pull request Apr 20, 2025

MAINT: Use numpy's Cython declarations for Iterator API. zoj613/polyagamma#131

Merged

	dtype = npyiter_get_common_dtype(nop, op,
	op_itflags, op_dtype,
	op_request_dtypes,
	only_inputs);
	if (dtype == NULL) {
	NpyIter_Deallocate(iter);
	return NULL;

Conversation

zoj613 commented Sep 13, 2023

Uh oh!

zoj613 commented Sep 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

seberg commented Sep 18, 2023

Uh oh!

seberg commented Sep 18, 2023

Uh oh!

zoj613 commented Sep 18, 2023

Uh oh!

zoj613 commented Sep 18, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zoj613 commented Oct 15, 2023

Uh oh!

seberg commented Oct 16, 2023

Uh oh!

zoj613 commented Oct 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

seberg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zoj613 commented Nov 11, 2023

Uh oh!

seberg commented Nov 16, 2023

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

zoj613 commented Sep 13, 2023 •

edited

Loading

zoj613 commented Oct 16, 2023 •

edited

Loading