Skip to content
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

ENH/MAINT: ndimage: implement LineBufferIterator class and wrappers. #7569

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jaimefrio
Copy link
Member

DO NOT MERGE

This is pretty large PR, with a proof of concept of the way I would like to refactor ndimage. It includes:

  • An implementation of NDIterator (very similar to NI_Iterator) and LineBufferIterator (somewhat similar to NI_LineBuffer), spread over three files (ni_iterators.h, ni_iterators.c and ni_iterator_utils.c). Note that the exposed API, see ni_iterators.h, is pretty small compared to the equivalent existing objects.

  • A Python wrapper for the LineBufferIterator object, see ni_iterator_pywrap.c. This would not be meant to be exposed to the user (the implementation right now is not very safe), but to enable in depth testing of the C code. As an example some tests have been added in test_ni_iterators.py.

  • An implementation of the uniform_filter1D function using these new iterators, also in ni_iterator_pywrap.c. This was done to demonstrate that:

    • Implementing a simple filter requires much less boilerplate than before, and
    • There is no performance degradation vs. the existing implementation.

I have used C in the last two points mostly for my convenience, but if we can agree that this is the right path forward, I would propose to:

  1. Create a separate PR to merge the C part of this PR, mostly as is.
  2. In a separate PR, add Cython wrappers to these two objects, along the lines of what is demonstrated here.
  3. In a third PR, add in depth testing, a.k.a. test the shit out, of the two iterators through the Cython wrappers.
  4. In a fourth PR, add a Cython module that implements all the functions in ni_filters.c that would use the new LineBufferIterator.
  5. In subsequent PRs, do something similar with NI_FilterIterator, although the details for that are still TBD.

@jaimefrio
Copy link
Member Author

There's no need for a detailed review of this code, this PR will be closed without merging. I'm mostly after comments on the general direction this is moving.

Copy link
Member

@stefanv stefanv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Jaime, thanks for working on improving the ndimage API. I completely trust your intuition on the best direction to take the code; we can't really go wrong either (it would take a lot to make the API worse ;).

npy_intp size_before, npy_intp size_after,
npy_double unused_value)
{
/* abcddcba|abcd|dcbaabcd */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment and the one in the function below are the same.

I remember there were issues with one of these, but not with which one.

@rgommers
Copy link
Member

rgommers commented Jul 9, 2017

Maybe not worth fixing if this PR is going to be closed, but just pointing out that the failures here are real:

ImportError: /home/travis/build/scipy/scipy/build/testenv/lib/python3.6/site-packages/scipy/
ndimage/_ni_iterators.cpython-36m-x86_64-linux-gnu.so: undefined symbol: Py_InitModule

Didn't look at why it only happens on a couple of the build configs.



LineBufferIterator*
LBI_New(PyArrayObject *input, int axis, PyArrayObject *output,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better rename this to something that doesn't have new in it - that usually doesn't make sense a few years after introduction.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is so good it will always feel like new... ;-)
It's supposed to be new as in new and delete in C++, i.e. the "class" constructor. It's inspired by the numpy iterator's NpyIter_New.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, that makes more sense:)

@rgommers
Copy link
Member

rgommers commented Jul 9, 2017

That looks like a good plan to me. The uniform_filter1D implementation is so much more readable than the old version, I like it!

@jaimefrio
Copy link
Member Author

The failures are in Python 3, I did not add the #ifs to handle module creation for that case.

@lucascolley lucascolley added enhancement A new feature or improvement needs-work Items that are pending response from the author needs-decision Items that need further discussion before they are merged or closed maintenance Items related to regular maintenance tasks labels Dec 23, 2023
@lucascolley lucascolley marked this pull request as draft March 14, 2024 21:57
@lucascolley lucascolley changed the title WIP: Implement LineBufferIterator class and wrappers. ENH/MAINT: ndimage: implement LineBufferIterator class and wrappers. Mar 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A new feature or improvement maintenance Items related to regular maintenance tasks needs-decision Items that need further discussion before they are merged or closed needs-work Items that are pending response from the author scipy.ndimage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants