[Numpy-discussion] Ready for review: PyArrayNeighIterObject, an iterator to iterate over a neighborhood in arbitrary arrays

[Charles R Harris]

On Sun, Jun 14, 2009 at 2:07 AM, David Cournapeau <cournape at gmail.com> wrote:
>
> On Sun, Jun 14, 2009 at 4:59 PM, David Cournapeau<cournape at gmail.com> wrote:
> > On Sun, Jun 14, 2009 at 3:51 AM, Charles R
> > Harris<charlesr.harris at gmail.com> wrote:
> >>
> >>
> >> On Sat, Jun 13, 2009 at 12:35 PM, David Cournapeau <cournape at gmail.com>
> >> wrote:
> >>>
> >>> On Sun, Jun 14, 2009 at 3:22 AM, Charles R
> >>> Harris<charlesr.harris at gmail.com> wrote:
> >>>
> >>> > 1) Since reference counting is such a pain, you should document that the
> >>> > constructor returns a new reference and that the PyArrayIterObject does
> >>> > not
> >>> > need to have its reference count incremented before the call and that
> >>> > the
> >>> > reference count is unchanged on failure.
> >>>
> >>> OK.
> >>>
> >>> > 2) Why are _update_coord_iter(c) and _inc_set_ptr(c) macros? Why are
> >>> > they
> >>> > defined inside functions? If left as macros, they should be in CAPS, but
> >>> > why
> >>> > not just write them out?
> >>>
> >>> They are macro because they are reused in the 2d specialized functions
> >>> (I will add 3d too)
> >>
> >> IIRC, inline doesn't recurse, so there is some advantage to having these as
> >> macros. But I really dislike seeing macros defined inside of functions,
> >> especially when they aren't exclusive to that function.
> >
> > Well, they are kind of exclusive to this function - and the special 2d
> > case; they are not supposed to be used by themselves (which is why
> > they are undefined right away). But I changed this anyway to ALL CAPS
> > and defined outside.
> >
> >> IOW, use macros judiciously.
> >
> > It may not be obvious, because it looks really simple, but the code is
> > heavily optimized. I spent several hours to find an implementation
> > which works as it does now. The macro is used for a reason :)
>
> Forgot a link to the updated version:
>
> http://github.com/cournape/scipy3/blob/1a4a6b4619a5d4f954168a02ed485db1a3b6b8e8/scipy/signal/neighiter.h

Looking good. A few more nitpicks ;)

1) The documentation of PyObject_Init doesn't say whether it is NULL
safe, so I think there needs to be a check here before the call:

ret = PyArray_malloc(sizeof(*ret));
PyObject_Init((PyObject *)ret, &PyArrayNeighborhoodIter_Type)
if (ret == NULL) {
    return NULL;
}

2) Do the bounds need to be ordered? If so, that should be mentioned
and checked.

3) In the documentation x is used but the function prototype uses iter.

4) I don't think the reference is borrowed since it is incremented if
the ctor succeeds. I think the point here is that the user doesn't
need to worry about it.

5) There should be spaces around the "-" here:
for (i = iter->nd-1; i >= 0; --i)
 Likewise, the convention in python seems to be a space between the
"for" and "("

6) If the functions use neighborhood (I do think that looks better),
then the file names should also.

Chuck