ENH: Nditer as context manager - #9998

Is this related to np.nditer? (edit: I see, separate commit)

Sorry if this is a naive question, I'm just coming back to this:

Why is this line needed? Doesn't the context manager exit take care of any needed writebacks? Furthermore, let's say that within the context manager the user creates a view of the array they got from nditer, and writes to the view (Thus calling Writeback) and then the context manager exits, doesn't that double-call the writeback code?

This PR segfaults on the following for me:

a = np.arange(24, dtype='f8').reshape(2, 3, 4).T
with np.nditer(a, [],[['readwrite', 'updateifcopy']],casting='same_kind',op_dtypes=[np.dtype('f4')]) as i:
    i.operands[0][...] = 3
    i.operands[0][...] = 14

which I suspect is related to my comment, but I haven't checked carefully.

good catch. Added your test and fixed in c6a1a2c6d09. The context manager does indeed clean up, but this function can possibly allocate a new ndarray with writebackifcopy semantics that need to be cleaned up. The fix was to call the resolve function iff a new view is created

Why not an enum?

Or even just a char for each flag - we don't care about efficiency here.

Perhaps

char has_writebackifcopy;
enum {
   CONTEXT_BEFORE,
   CONTEXT_INSIDE,
   CONTEXT_AFTER
} context_state;

fixed in 9a45b3d055

Would be better as npyiter_enter since this also sets flags.

fixed in 9a45b3d055

Should this be |=?

refactored in 9a45b3d055

Why does this clear ITR_HAS_WRITEBACKIFCOPY?

IIUC, PyArray_ResolveWritebackIfCopy is called, so there's no need to worry about writing back.

So what does the following do ?

it = np.nditer(...)
with it:
    next(it)
with it:
   # do stuff

It seems to me that we'll forget that we were supposed to be in a context manager the second time around

That's currently undefined behavior.

Obviously that should be fixed by moving the constructor logic into __enter__.

IMO calling __enter__ the second time should raise an exception since writebackifcopy resolution would have already been triggered. Is it common to try to re-enter a context manager? In my limited experience I have not seen code like that

Raising an exception sounds reasonable to me - as long as it doesn't fail silently

now raises an error, tested in 9a45b3d055

Braces and/or indentation needed here

fixed in 9a45b3d055

Why not declare f as NewNpyArrayIterObject like you do in npy_iter_self?

fixed in 9a45b3d055

Why is this desirable? How do operands interact with WRITEBACK_IF_COPY? Do they contain the temporary, or the target of the writeback?

IIUC, you must call PyArray_ResolveWritebackIfCopy only once. I think operands contain the temporary copy, they operate on it, and then when the context has exited, the results are written back. Obviously they cannot be written back if the context has already exited.

Not only that, but once they have been written back, the temporary scratch buffer is removed. Now the whole reason for the writeback semantics have been undone, for instance the operand dtype conflicts with the underlying base array's dtype. Reading from the operand will most likely retrieve the wrong value, or writing to it will corrupt the data.

Would be ideal to recreate the scratch buffer in every __enter__. But fixing the deprecation is a higher priority IMHO.

So to be clear, operands contains the temporary scratch buffer, not the original array?

I think operands contain the temporary copy

operands are ndarrays that have the original array as a base attribute. Since they have the WRITEBACKIFCOPY flag, they hold another scratch buffer until PyArray_ResolveWritebackIfCopy is called

del it might be clearer here

fixed in 9a45b3d055

Why not with i, j?

That's one possibility. I think people were looking for a one-line solution and Python 3 may require a two-line solution.

I don't have a good enough understanding of nested_iters to know what makes sense here. Is it documented anywhere? Do the nditers it returns depend on each other? If so, isn't it problematic to construct the inner iter before calling __enter__ on the outer one? (which is exactly the reason that python 3 removed contextlib.nested)

It is part of the public API, but it isn't documented.

It would be nice if it was at least internally documented. Right now, I have very little idea of what it does.

I would prefer to resolve the nested_iter issue in a separate pull request. I have filed issue #10009 a a reminder to do so. AFAICT the c-api function NpyIter_NestedIters is not part of the public API, so we could fix this on the python level only

heh, in the end it is enough to do what @eric-wieser suggested in the first comment, simple and clear. I left the issue open since the function is not documented

Unfortunately, AFAICT there are circumstances where nditer can use UPDATEIFCOPY even if "updateifcopy" isn't passed, like if you have a writable array that overlaps with another array.

Suggested text:

Under certain conditions, nditer must be used in a context manager
-----------------------------------------------
When using an nditer with the `"writeonly"` or `"readwrite"` flags, there are some
circumstances where nditer doesn't actually give you a view onto the writable
array. Instead, it gives you a copy, and if you make changes to the copy, nditer later
writes those changes back into your actual array. Currently, this writeback depends
on when the array objects are garbage collected, which makes this API error-prone
on CPython and entirely broken on PyPy. Therefore, `nditer` can now be used
as a context manager (`with np.nditer(...) as it: ...`), and in the future this will
be required whenever using `nditer` with writable arrays.

Note also that I'm suggesting here that we actually make the criterion for issuing deprecation warnings (now) and raising an error (later) be no context manager + writable arrays, since that's so much easier to explain to users than the actual edge cases. (And in particular if you have writable arrays then it's easy to get into a situation where in testing you never hit the UPDATEIFCOPY path, but then in production...)

I tried to couch the wording in a way that did not limit it to the single flag. Requiring any writable nditer to be used inside a context manager pretty much means any nditer must be used that way. Is a change that deep acceptable/desired?

The problem is that anyone who uses writable nditer might need this depending on details of the input arrays memory layout (in particular whether they overlap). I think forcing everyone to change at once with a nice warning message is better than forcing people to change piecemeal as they accidentally hit the edge case one by one.

Fortunately almost no-one uses nditer :-)

I went through all the hits for nditer on searchcode.com, and only found ~15 projects, of which only ~5 use writeonly/readwrite. I'm sure there are others not in their corpus, but we're still only talking about probably ~dozens of pieces of code worldwide that need updating.

fixed in 7eb8aed72

If array_dealloc sees WRITEBACKIFCOPY then that's just a bug, right? Shouldn't we issue a RuntimeWarning or something? (Really it ought to raise an exception, but since you can't do that from array_dealloc a noisy warning is a reasonable alternative.)

fixed in 7e5c2a7e6, warning message adjusted to indicate the root cause could be nditer

Isn't this just our PyUString_FromString macro?

I'm assuming view can be NULL here from the XDECREF below. Is this save if view is NULL?

PyArray_DiscardWritebackIfCopy(arr) does check if arr is NULL

Why do you clear this?

This second condition is always true, right, because there's no way to be inside the context already here?

gaah, this is a bug. I need to check the operands and set iter->has_writebackifcopy appropriately

Nit: elsewhere we write this 2017-11-10 or 2017-Nov-10

Should this call Resolve even if an error occurs? (args doesn't contain Py_Nones)

Or would it be better to Discard?

correct, I did not properly handle the error condition. TODO

fixed in 0c3340936f3

extra blank line here

also, all lines related to needs_context_manager can be removed now.

fixed