Skip to content

MAINT: BUG: review need for NpyIter_Close, add missing test for out #11370

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

Closed
wants to merge 1 commit into from

Conversation

mattip
Copy link
Member

@mattip mattip commented Jun 18, 2018

After reviewing all the places NpyIter_Dealloc is called without NpyIter_Close, it seems the only case that can trigger the need to call NpyIter_Close (reminder: that is needed when an operand has been created with the WRITEBACK_IF_COPY flag) is in one of the compiled test modules.

I left comments next to the calls to NpyIter_Dealloc as reminders and for reviewers.

I also added a passing test for the out argument in np.busday_offset, see issue #11369 about test coverage

@eric-wieser
Copy link
Member

Can you remind me why you introduced NpyIter_Close() at all? Couldn't the cleanup always be done by NpyIter_Deallocate()?

Couldn't np.nditer.close() have called NpyIter_Deallocate(), and just set the internal iterator pointer to null?

@@ -1112,6 +1112,7 @@ arr_ravel_multi_index(PyObject *self, PyObject *args, PyObject *kwds)
Py_XDECREF(op[i]);
}
npy_free_cache_dim_obj(dimensions);
/* no NpyIter_Close required since output is always allocated */
Copy link
Member

Choose a reason for hiding this comment

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

Is there any reason not to just always call it? Testing a handful of flags should be pretty cheap, and if its not, you could cache them in the constructor?

Copy link
Member Author

Choose a reason for hiding this comment

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

We store the need to resolve the writeback in a per-operand flag, so traversing those would be very cheap.

This PR is more to check if our testing is adequate, if there were places that are missing a needed NpyIter_Close that would indicate missing tests. It seems that part is OK, barring mistakes.

@mattip
Copy link
Member Author

mattip commented Jun 18, 2018

When we discussed the design there was a need for

with np.iter(...) as it:
    # do things with the iterator
    a = it.operands[0]
print(a)

NpyIter_Close is called at the context manager's exit. We could do the other way around - detect/warn/call NpyIter_Close inside NpyIter_Dealloc

Edit: formatting

@eric-wieser
Copy link
Member

eric-wieser commented Jun 18, 2018

NpyIter_Close is called at the context manager's exit.

Why can't we just call NpyIter_Dealloc at the context manager's exit?

@mattip
Copy link
Member Author

mattip commented Jun 18, 2018

If we merge NpyIter_Dealloc and NpyIter_Close, npyiter_dealloc would need to warn if any operand flags have NPY_OP_ITFLAG_HAS_WRITEBACK. We should decide before the 1.15 release since NpyIter_Close is part of the public API.

@eric-wieser
Copy link
Member

eric-wieser commented Jun 18, 2018

npyiter_dealloc would need to warn if any operand flags have NPY_OP_ITFLAG_HAS_WRITEBACK

Doesn't it already do that if you don't call it.close().

Just to be clear here:

Python C slot implementation currently proposed
it.close() npyiter_close NpyIter_Close NpyIter_Dealloc, null internal pointer
del it npyiter_dealloc NpyIter_Dealloc, warning if close was needed but not called if internal pointer not already null, NpyIter_Dealloc (and warn?)

@mattip
Copy link
Member Author

mattip commented Jun 18, 2018

There was some desire in the original discussion to allow re-entering the iterator:

a = np.arange(12)
it = np.nditer(a)
with it:
    with it:
        for x in it:
            print(x)

If we call NpyIter_Deallocate on exit this will raise when the second __exit__ is called.

@mattip
Copy link
Member Author

mattip commented Jun 18, 2018

@ahaldane any thoughts? If we disallow the reentry, this seems like a nice cleanup. In any case, it seems we can remove NpyIter_Close from the external c-api and keep it only for internal use by npyiter_close. Any c-api user will call NpyIter_Dealloc which will silently resolve writebacks without an explicit call to NpyIter_Close.

@eric-wieser
Copy link
Member

this will raise when the second exit is called.

There's no reason for this to be true - it.__exit__ can just be a no op if the internal NpyIter is null.

@mattip
Copy link
Member Author

mattip commented Jun 18, 2018

Found a way to raise the warning even if the iterator is reentrant in #11376

@mattip
Copy link
Member Author

mattip commented Jun 19, 2018

PR #11376 replaces this

@mattip mattip closed this Jun 19, 2018
@mattip mattip deleted the review-nditer branch March 4, 2019 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants