DOC: cleanup documentation, continuation of nditer PR #9998 #10949

mattip · 2018-04-21T22:45:42Z

After merging PR #9998, careful review picked up some mistakes in documentation

eric-wieser · 2018-04-21T22:48:39Z

doc/release/1.15.0-notes.rst

@@ -118,7 +118,7 @@ C API changes
 =============

 ``NpyIter_Close`` has been added and should be called before
-``NpyIter_Dealloc`` to resolve possible writeback-enabled arrays.
+``NpyIter_Deallocate`` to resolve possible writeback-enabled arrays.


Would be tempted to add a bullet point to match the other sections without headers in this document.

eric-wieser · 2018-04-21T22:49:35Z

numpy/core/tests/test_nditer.py

+    with it:
+        with it:
+            for x in it:
+                x[...] = 123


What should happen if the outer with continues to use it?

it will error out that it is closed

That seems to be consistent with file and contextlib.closing. Can you test that too then?

eric-wieser · 2018-04-21T23:24:27Z

numpy/core/tests/test_nditer.py

    with it:
        for x in it:
            x[...] = 123
-    # make sure we cannot reenter the iterand
+    # make sure we cannot reenter the closed iterand


I'd argue this should be iterator, not iterand - I'd call it.operands the iterands.

mattip · 2018-04-21T23:33:16Z

squashed after last fix from review

ahaldane · 2018-04-22T00:16:11Z

In addition to these fixups, I think I had in mind some other additions to the docs which would be good to put in this PR.... I need to go back and review what those were

ahaldane · 2018-04-22T00:55:23Z

doc/source/reference/arrays.nditer.rst

@@ -78,6 +78,8 @@ order='C' for C order and order='F' for Fortran order.
    ...
    0 3 1 4 2 5

+.. _nditer-context-manager:
+
 Modifying Array Values
 ----------------------



The next few paragraph below this point could use some editing. It uses the term "view" in a possbily misleading way, since the writeback arrays are not actually views, they are temp buffers.

Here's my try going through it:

By default, the :class:`nditer` treats the input operand as a read-only object. To be able to modify the array elements, you must specify either read-write or write-only mode using the `'readwrite'` or `'writeonly'` per-operand flags. The nditer will then yield writeable buffer arrays which you may modify. However, because the nditer must copy this buffer data back to the original array once iteration is finished, you must signal when the iteration is ended, by one of two methods. You may either: - used the nditer as a context manager using the `with` statement, and the temporary data will be written back when the context is exited. - call the iterator's `close` method once finished iterating, which will trigger the write-back. The nditer can no longer be iterated once either `close` is called or its context is exited. When assigning to the buffers returned by the nditer, also remember that you must perform the assignment using the ellipsis index, as in `x[...] = `, rather than simply reassigning the variable name as in `x = `. [Note: Consider deleting this last paragaph entirely?]

ahaldane · 2018-04-22T01:03:06Z

Also some tweaks in doc/source/reference/c-api.iterator.rst: There is a typo shuold in NpyIter_Deallocate, and maybe in the doc for NpyIter_Close replace the word "must" by "should".

I was originally thinking of adding more wordy description of the need for Npy_Close there, but looking again that file is not very explanatory, it basically just says "look at the numpy code", and I think it's fine to leave it that way.

ahaldane · 2018-04-22T01:12:35Z

I guess maybe in the NpyIyer_Close doc, maybe add a sentence about how if you make copies using NpyIter_Copy, you need to Npy_Dealloc all of them, but you should only call NpyIter_Close once since they share buffers.

I think that covers all the doc changes I had in mind.

mattip · 2018-04-22T07:21:38Z

Also see #10951 which adds more documentation for it.close

charris · 2018-04-23T17:23:58Z

Needs a rebase.

mattip · 2018-04-23T19:35:13Z

rebased off master and squashed

eric-wieser · 2018-04-25T05:35:58Z

doc/source/reference/c-api.iterator.rst

-    ``NpyIter_Deallocate``. After this call it is not safe to use the operands.
+    Resolves any needed writeback resolution. Should be called before
+    :c:func::`NpyIter_Deallocate`. After this call it is not safe to use the operands.
+    When using :c:func:`NpyIter_Copy`, only one call to :c:func:`NpyIter_Close`


Is NpyIter_Copy exposed to python?

yes, as it.copy(). Should I expand the documentation of it.copy to mention writeback considerations?

Won'y np.nditer.__del__ complain if you only call close on the original and not the copy?

no, the complaint is on the operands, which are shared:

def test_copy(): a = np.arange(6, dtype='f4') au = a.byteswap().newbyteorder() it1 = np.nditer(au, [], [['readwrite', 'updateifcopy']], casting='equiv', op_dtypes=[np.dtype('f4')]) it2 = it1.copy() x = it1.operands[0] assert x is it2.operands[0] assert x.flags.writebackifcopy is True it1.close() assert x.flags.writebackifcopy is False

commenting the last two lines does emit a warning

So requiring a context manager wouldn't have worked here anyway?

Not sure what you mean. The context manager resolves operands on exit:

def test_copy(): a = np.arange(6, dtype='f4') au = a.byteswap().newbyteorder() it1 = np.nditer(au, [], [['readwrite', 'updateifcopy']], casting='equiv', op_dtypes=[np.dtype('f4')]) with it1: it2 = it1.copy() x = it1.operands[0] assert x is it2.operands[0] assert x.flags.writebackifcopy is True assert x.flags.writebackifcopy is False # no warning on it2 deallocate, the operand is resolved via it1

Note that nesting the context managers, it1 is still open even though it2 is closed

def test_copy(): a = np.arange(6, dtype='f4') au = a.byteswap().newbyteorder() it1 = np.nditer(au, [], [['readwrite', 'updateifcopy']], casting='equiv', op_dtypes=[np.dtype('f4')]) with it1: with it1.copy() as it2: x = it1.operands[0] assert x is it2.operands[0] assert x.flags.writebackifcopy is True assert x.flags.writebackifcopy is False y = it1.operands[0] # can still access it1's operand, it is not closed assert y is x

Why do we allow access to it1.operands and forbid access to it2.operands on that line, when they point to the same object? Either they're both invalid, or neither of them are invalid.

Ie, hasn't the writeback already happened, in which case it.operands is now temporary garbage that won't ever make it back to au?

It seems like preventing access to it.operands after it.close() is called has failed to actually protect against access to a post-writeback array (and so there's no point in imposing the restriction at all)

Disallowing new access to operands is a best-effort attempt, and is not fool-proof. x also no longer points to au after writeback resolution. Issue #10956 proposed a change to deal with that. This PR was meant to document the way it works now, with the warts.

Yep, this PR looks good to me as a doc improvement, but I'm worried that writebackifcopy might have been the wrong design after all. In my opinion, you should have to call close on both iterators for writebacks to be resolved, which would require a reference count.

note added to issue #10958

eric-wieser · 2018-04-27T06:55:37Z

@ahaldane, I'm going to leave this for you to merge

ahaldane · 2018-04-27T17:04:04Z

All right, looks good. Thanks Matti!

eric-wieser reviewed Apr 21, 2018

View reviewed changes

eric-wieser approved these changes Apr 21, 2018

View reviewed changes

mattip force-pushed the doc-nditer branch 2 times, most recently from 11c71b9 to 2e3e96b Compare April 21, 2018 23:32

ahaldane reviewed Apr 22, 2018

View reviewed changes

mattip force-pushed the doc-nditer branch 2 times, most recently from a5c2458 to c410fdc Compare April 22, 2018 07:19

eric-wieser added the 04 - Documentation label Apr 22, 2018

DOC: cleanup documentation, continuation of nditer PR numpy#9998

86608c2

mattip force-pushed the doc-nditer branch from c410fdc to 86608c2 Compare April 23, 2018 19:34

Merge branch 'master' into doc-nditer

246ad1d

eric-wieser reviewed Apr 25, 2018

View reviewed changes

ahaldane merged commit 0a02ea2 into numpy:master Apr 27, 2018

mattip deleted the doc-nditer branch May 6, 2018 13:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DOC: cleanup documentation, continuation of nditer PR #9998 #10949

DOC: cleanup documentation, continuation of nditer PR #9998 #10949

mattip commented Apr 21, 2018

eric-wieser Apr 21, 2018

mattip Apr 21, 2018

eric-wieser Apr 21, 2018

mattip Apr 21, 2018

eric-wieser Apr 21, 2018

mattip Apr 21, 2018

eric-wieser Apr 21, 2018

mattip Apr 21, 2018

mattip commented Apr 21, 2018

ahaldane commented Apr 22, 2018

ahaldane Apr 22, 2018 •

edited

Loading

ahaldane commented Apr 22, 2018

ahaldane commented Apr 22, 2018

mattip commented Apr 22, 2018

charris commented Apr 23, 2018

mattip commented Apr 23, 2018

eric-wieser Apr 25, 2018

mattip Apr 25, 2018

eric-wieser Apr 25, 2018

mattip Apr 25, 2018

eric-wieser Apr 25, 2018

mattip Apr 25, 2018

eric-wieser Apr 25, 2018 •

edited

Loading

mattip Apr 25, 2018

eric-wieser Apr 25, 2018

mattip Apr 25, 2018

eric-wieser commented Apr 27, 2018

ahaldane commented Apr 27, 2018

DOC: cleanup documentation, continuation of nditer PR #9998 #10949

DOC: cleanup documentation, continuation of nditer PR #9998 #10949

Conversation

mattip commented Apr 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattip commented Apr 21, 2018

ahaldane commented Apr 22, 2018

ahaldane Apr 22, 2018 • edited Loading

Choose a reason for hiding this comment

ahaldane commented Apr 22, 2018

ahaldane commented Apr 22, 2018

mattip commented Apr 22, 2018

charris commented Apr 23, 2018

mattip commented Apr 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eric-wieser Apr 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eric-wieser commented Apr 27, 2018

ahaldane commented Apr 27, 2018

ahaldane Apr 22, 2018 •

edited

Loading

eric-wieser Apr 25, 2018 •

edited

Loading