ENH: Nditer as context manager #9998

mattip · 2017-11-10T13:04:50Z

Resolves issue #9714 by enabling use of nditer as a context manager. Code like this:

a = np.arange(24, dtype='f8').reshape(2, 3, 4).T
it = np.nditer(a, [], [['readwrite', 'updateifcopy']],
            casting='same_kind', op_dtypes=[np.dtype('f4')])
# Check that UPDATEIFCOPY is activated
it.operands[0][2, 1, 1] = -12.5
assert  a[2, 1, 1] != -12.5
it = None                     # magic!!!
assert a[2, 1, 1] == -12.5

now becomes

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 it:
    # Check that UPDATEIFCOPY is activated
    it.operands[0][2, 1, 1] = -12.5
    assert  a[2, 1, 1] != -12.5
assert a[2, 1, 1] == -12.5

No more need to do the it = None assignment to write the data back to the original array. Tests were adjusted. In addition, DeprecationWarning will be raised if a nditer should be using this code pattern and is not, or if the nditer it is used outside the context manager.

This also allows completing the confusing part of pull request #9639, and now any call to PyArray_SetUpdateIfCopyBase will emit a DeprecationWarning.

I am not really happy with the way this affects nested_iters, since it returns a tuple there is no convienient way to handle the context manager there, that is why the test tweak is in a seperate commit.

ahaldane · 2017-11-10T17:32:38Z

Is it possible to create a simple custom context manager for nested_iters?
Something like:

class nested_iters(object):
    def __init__(self, args):
        self.mgr1 = nditer(args, 1)
        self.mgr2 = nditer(args, 2)

    def __enter__(self):
        return (self.mgr1.__enter__(), self.mgr2.__enter__())

    def __exit__(self, type, value, tb):
        self.mgr1.__exit__()
        self.mgr2.__exit__()

It may also be easier do this in python instead of trying to modify NpyIter_NestedIters in C - I'm not sure any C-level control is needed, though I'm not too familiar with the code. Probably it could be stuck into numpy/core/numeric.py. It doesn't look like NpyIter_NestedIters is part of the C-api, so we wouldn't even need a C interface. (If it is, we can put the python definition in numpy/core/_internals.py and call it from C with npy_cache_import).

(incidentally, nested_iters is not documented in the docs, we should fix that some day)

ghost · 2017-11-11T18:43:04Z

IHMO just reimplement nested for python 3 and return that:

try:
    from contextlib import nested  # Python 2
except ImportError:
    from contextlib import ExitStack, contextmanager

    @contextmanager
    def nested(*contexts):
        """
        Reimplementation of nested in python 3.
        """
        with ExitStack() as stack:
            for ctx in contexts:
                stack.enter_context(ctx)
            yield contexts

ghost · 2017-11-11T18:44:46Z

Rare case where Python 2 has a better API.

eric-wieser · 2017-11-12T00:55:51Z

numpy/core/src/multiarray/mapping.c

@@ -2143,6 +2144,7 @@ array_assign_subscript(PyArrayObject *self, PyObject *ind, PyObject *op)
    return -1;

  success:
+    PyArray_ResolveWritebackIfCopy(view);


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

eric-wieser · 2017-11-12T00:56:56Z

numpy/core/src/multiarray/nditer_pywrap.c

+    /* iter must used as a context manager if writebackifcopy semantics used */
+    #define ITR_INSIDE 1<<0
+    #define ITR_HAS_WRITEBACKIFCOPY 1<<1
+    #define ITR_EXITED 1<<2


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

eric-wieser · 2017-11-12T00:58:16Z

numpy/core/src/multiarray/nditer_pywrap.c

@@ -2331,6 +2381,39 @@ npyiter_ass_subscript(NewNpyArrayIterObject *self, PyObject *op,
    return -1;
 }

+static PyObject *
+npyiter_self(NewNpyArrayIterObject *self)


Would be better as npyiter_enter since this also sets flags.

fixed in 9a45b3d055

eric-wieser · 2017-11-12T00:58:52Z

numpy/core/src/multiarray/nditer_pywrap.c

+        PyErr_SetString(PyExc_ValueError, "operation on  non-initialized iterator");
+        return NULL;
+    }
+    self->managed &= ITR_INSIDE;


Should this be |=?

refactored in 9a45b3d055

eric-wieser · 2017-11-12T00:59:45Z

numpy/core/src/multiarray/nditer_pywrap.c

+        if (PyArray_ResolveWritebackIfCopy(*object) < 0)
+        return NULL;
+    }
+    self->managed = ITR_EXITED;


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

eric-wieser · 2017-11-12T01:00:34Z

numpy/core/src/multiarray/nditer_pywrap.c

+    object = NpyIter_GetOperandArray(iter);
+    for(iop = 0; iop < nop; ++iop, ++object) {
+        if (PyArray_ResolveWritebackIfCopy(*object) < 0)
+        return NULL;


Braces and/or indentation needed here

fixed in 9a45b3d055

eric-wieser · 2017-11-12T01:01:12Z

numpy/core/src/multiarray/nditer_pywrap.c

+}
+
+static PyObject *
+npyiter_exit(PyObject *f, PyObject *args)


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

fixed in 9a45b3d055

eric-wieser · 2017-11-12T01:04:25Z

numpy/core/src/multiarray/nditer_pywrap.c

+        PyErr_SetString(PyExc_ValueError,
+                "Iterator has exited its context");
+        return NULL;
+    }


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

eric-wieser · 2017-11-12T01:05:11Z

numpy/core/tests/test_nditer.py

+        assert_(not it.iterationneedsapi)
+        assert_(sys.getrefcount(a) > rc_a)
+        assert_(sys.getrefcount(dt) > rc_dt)
+    # 'it' is still alive in this scope, dealloc


del it might be clearer here

fixed in 9a45b3d055

eric-wieser · 2017-11-12T01:09:27Z

numpy/core/tests/test_nditer.py

@@ -2284,7 +2284,8 @@ def test_iter_nested_iters_dtype_copy():
    assert_equal(vals, [[0, 1, 2], [3, 4, 5]])
    vals = None

-    # updateifcopy
+    # writebackifcopy
+    # XXX ugly - is there a better way? np.nested_iter returns a tuple


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

njsmith · 2017-11-12T05:04:30Z

doc/release/1.14.0-notes.rst

+``dealloc`` call, which let to strange ``i = None`` code after iteration to
+magically write the temporary data back to the original source. Now nditers
+can be used as context managers, and when writebackoncopy semantics are
+triggered they *must* be used as context managers.


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

njsmith · 2017-11-12T05:07:43Z

doc/release/1.14.0-notes.rst

+
+``DeprecationWarning`` warnings will be issued on use of
+``UPDATEIFCOPY`` or if writeback resolution has not
+occurred before calling ``array_dealloc``.


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

eric-wieser · 2017-11-12T22:17:58Z

numpy/core/src/multiarray/arrayobject.c

+# define MSG_FromString PyString_FromString
+#else
+# define MSG_FromString PyUnicode_FromString
+#endif


Isn't this just our PyUString_FromString macro?

eric-wieser · 2017-11-12T22:19:19Z

numpy/core/src/multiarray/mapping.c

@@ -2134,6 +2134,7 @@ array_assign_subscript(PyArrayObject *self, PyObject *ind, PyObject *op)

    /* Clean up temporary variables and indices */
  fail:
+    PyArray_DiscardWritebackIfCopy(view);


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

eric-wieser · 2017-11-12T22:20:29Z

numpy/core/src/multiarray/nditer_pywrap.c

+    for (iop = 0; iop < nop; ++iop) {
+        if (op_flags[iop] & NPY_ITER_USE_CONTEXT_MANAGER)
+            self->has_writebackifcopy = 1;
+        op_flags[iop] &= ~NPY_ITER_USE_CONTEXT_MANAGER;


Why do you clear this?

eric-wieser · 2017-11-12T22:22:16Z

numpy/core/src/multiarray/nditer_pywrap.c

@@ -1065,6 +1088,12 @@ NpyIter_NestedIters(PyObject *NPY_UNUSED(self),
            Py_DECREF(ret);
            goto fail;
        }
+        if ((iter->has_writebackifcopy != 0) &&
+            (iter->managed != CONTEXT_INSIDE)) {


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

eric-wieser · 2017-11-12T22:23:15Z

numpy/core/src/multiarray/nditer_pywrap.c

-
+    if ((self->has_writebackifcopy != 0) &&
+        (self->managed != CONTEXT_INSIDE)) {
+        /* 10-Nov-2017 v1.14 */


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

eric-wieser · 2017-11-12T22:24:32Z

numpy/core/src/multiarray/nditer_pywrap.c

+    nop = NpyIter_GetNOp(iter);
+    object = NpyIter_GetOperandArray(iter);
+    for(iop = 0; iop < nop; ++iop, ++object) {
+        if (PyArray_ResolveWritebackIfCopy(*object) < 0) {


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

eric-wieser · 2017-11-13T08:27:39Z

One overall comment - it's very rare to have something in python that is required to be a context manager - for instance, the file objects provide a close method for when you don't use with.

Perhaps we should add commit_writeback and discard_writeback methods, and leave with as just syntactic sugar? I think that would be better because:

It's easier to adapt old code for the rare cases when with is messy (ie, those that require ExitStack
We don't have a real __enter__ method anyway
It makes it closer to the C code that np.nditer is supposed to be a learning tool to help use

mattip · 2017-11-13T19:33:43Z

I have no strong opinion on commit_writeback and discard_writeback, but then we would need another function to indicate "I promise to call them later", like allow_nditer_outside_context_manager which is a really bad name. This function would set the needs_context_manager flag to 0.

ahaldane · 2018-04-20T15:59:41Z

All right. I'm going to do one last pass through the code soon, and then I am aiming to merge sometime in the next few days.

ahaldane · 2018-04-21T18:59:26Z

numpy/core/src/multiarray/nditer_pywrap.c

+        if (op_flags[iop] & NPY_ITER_READWRITE) {
+            self->needs_context_manager = 1;
+        }
+


extra blank line here

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

ahaldane · 2018-04-21T19:02:45Z

All right, I did my pass. Just a few nits, and removal of needs_context_manager.

After you fix those last few, I will merge.

mattip · 2018-04-21T19:59:10Z

Thanks, this fixes #9714 and is the last stage to resolve #7054

ahaldane · 2018-04-21T20:38:21Z

All right! Here goes....

Thank you so much for bearing with all the reviews, I think it is quite a nice PR now.

eric-wieser · 2018-04-21T20:56:18Z

numpy/core/code_generators/numpy_api.py

@@ -5,7 +5,8 @@

 Whenever you change one index, you break the ABI (and the ABI version number
 should be incremented). Whenever you add an item to one of the dict, the API
-needs to be updated.
+needs to be updated in both setup_common.py and by adding an appropriate
+entry to cversion.txt (generate the hash via "python cversions.py".


nit: missing paren

What is "best practice" when fixing a merged PR, a new PR or modifying this one again?

You can't modify an existing PR once it's merged.

What I normally do is keep working in the same branch, but open a new PR

fixed (in doc-nditer PR)

eric-wieser · 2018-04-21T20:56:34Z

numpy/add_newdocs.py

+    """
+    close()
+
+    Resolve all writeback semantics in writeable operands.


Perhaps reference the with statement here.

fixed (in doc-nditer PR)

eric-wieser · 2018-04-21T21:06:30Z

doc/release/1.15.0-notes.rst

@@ -93,6 +117,8 @@ using the old API.
 C API changes
 =============

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


Should this be NpyIter_Deallocate?

fixed (in doc-nditer PR)

eric-wieser · 2018-04-21T21:09:57Z

numpy/core/tests/test_nditer.py

    assert_equal(au, [2]*6)
-
+    i = None # should not raise a DeprecationWarning


nit: PEP8 wants two spaces before the #. Also, del i might be clearer.

fixed (in doc-nditer PR)

eric-wieser · 2018-04-21T21:25:29Z

numpy/core/src/multiarray/arrayobject.c

-#ifdef DEPRECATE_UPDATEIFCOPY 
-    /* TODO: enable this once a solution for UPDATEIFCOPY
-     *  and nditer are resolved, also pending the fix for GH7054
-     */
    /* 2017-Nov-10 1.14 */


I think this needs an additional comment for the non-pypy deprecation

What would you suggest? Leaving the "GH7054" issue reference as a hint?

/* 2017-11-10 1.14 (only in pypy) */ /* 2018-04-21 1.15 (all python implementations) */

eric-wieser · 2018-04-21T21:31:57Z

numpy/core/src/multiarray/nditer_pywrap.c

 struct NewNpyArrayIterObject_tag {
    PyObject_HEAD
    /* The iterator */
    NpyIter *iter;
    /* Flag indicating iteration started/stopped */
    char started, finished;
+    /* iter must used as a context manager if writebackifcopy semantics used */
+    char managed;


The comment above this line is no longer correct, right?

eric-wieser · 2018-04-21T21:33:04Z

numpy/core/src/multiarray/nditer_pywrap.c

+        PyErr_SetString(PyExc_ValueError, "cannot reuse iterator after exit");
+        return NULL;
+    }
+    self->managed = CONTEXT_INSIDE;


Should probably also check that you're not re-entering the iterator

Either that, or allow re-entrant withs, but only cleanup on the outermost one.

Reentrant is OK (test added), multiple cleanups are harmless. Once the cleanup is finished, we set self->managed = CONTEXT_EXITED so reentering a closed nditer fails (there is a test for that in test_nditer.py, line 2847

I guess the question is, is this the behavior we expect?

with it: with it: do_stuff_with_it() # ok do_stuff_with(it) # error: already closed

It does seem to match how contextlib.closing works, so I suppose is consistent.

eric-wieser · 2018-04-21T21:42:14Z

numpy/core/src/multiarray/nditer_api.c

+ * Returns 0 on success, -1 on failure
+ */
+NPY_NO_EXPORT int
+NpyIter_Close(NpyIter *iter)


Is it safe to call this multiple times?

the first call will resolve all writebacks, so if there was a writeback scratch buffer now the iterator operand's base attribute will be None and the operand's data will be independent from the original writeable array data. Subsequent calls will no do anything

eric-wieser · 2018-04-21T22:55:59Z

numpy/core/src/multiarray/nditer_pywrap.c

@@ -1473,6 +1489,12 @@ static PyObject *npyiter_itviews_get(NewNpyArrayIterObject *self)
        return NULL;
    }

+    if (self->managed == CONTEXT_EXITED) {
+        PyErr_SetString(PyExc_ValueError,
+                "Iterator is closed");


Shouldn't this happen after .close() is called, not just after __exit__ is called?

yes, see new PR for issue #10950

DOC: cleanup documentation, continuation of nditer PR #9998

mattip force-pushed the nditer-as-context-manager branch from a68c7ef to 12d6caf Compare November 10, 2017 14:09

This was referenced Nov 10, 2017

change updateifcopy semantics in nditer #9714

Closed

UPDATEIFCOPY breaks on python interpreters with non-refcounted GC #7054

Closed

charris added 01 - Enhancement component: numpy._core labels Nov 10, 2017

eric-wieser reviewed Nov 12, 2017

View reviewed changes

njsmith reviewed Nov 12, 2017

View reviewed changes

mattip mentioned this pull request Nov 12, 2017

DOC: nditer.nested_iters are not documented #10009

Closed

eric-wieser reviewed Nov 12, 2017

View reviewed changes

mattip force-pushed the nditer-as-context-manager branch from 4299e5e to 7eb8aed Compare November 13, 2017 07:48

mattip force-pushed the nditer-as-context-manager branch from 1e152b4 to 38f7a34 Compare April 20, 2018 09:07

ENH: add nditer.close as per review

c26d273

mattip force-pushed the nditer-as-context-manager branch from 38f7a34 to c26d273 Compare April 20, 2018 10:11

ahaldane reviewed Apr 21, 2018

View reviewed changes

fixes from review

fa9a741

mattip force-pushed the nditer-as-context-manager branch from 94730b0 to fa9a741 Compare April 21, 2018 19:57

ahaldane merged commit e0b5e87 into numpy:master Apr 21, 2018

eric-wieser reviewed Apr 21, 2018

View reviewed changes

mattip mentioned this pull request Apr 21, 2018

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

Merged

eric-wieser reviewed Apr 21, 2018

View reviewed changes

mattip mentioned this pull request Apr 21, 2018

BUG: it.close() disallows access to iterator, fixes #10950 #10951

Merged

mattip added a commit to mattip/numpy that referenced this pull request Apr 23, 2018

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

86608c2

mattip deleted the nditer-as-context-manager branch April 24, 2018 09:17

ahaldane added a commit that referenced this pull request Apr 27, 2018

Merge pull request #10949 from mattip/doc-nditer

0a02ea2

DOC: cleanup documentation, continuation of nditer PR #9998

mattip mentioned this pull request Jun 18, 2018

ENH: Remove NpyIter_Close #11376

Merged

charris mentioned this pull request Jun 20, 2018

ENH: Remove NpyIter_Close #11389

Merged

		assert_equal(au, [2]*6)

		i = None # should not raise a DeprecationWarning

ENH: Nditer as context manager #9998

ENH: Nditer as context manager #9998

Conversation

mattip commented Nov 10, 2017

ahaldane commented Nov 10, 2017 • edited

ghost commented Nov 11, 2017

ghost commented Nov 11, 2017

eric-wieser Nov 12, 2017 • edited

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 Nov 12, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eric-wieser Nov 12, 2017 • edited

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eric-wieser Nov 12, 2017 • edited

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

njsmith Nov 12, 2017 • edited

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

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

eric-wieser Nov 12, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eric-wieser commented Nov 13, 2017 • edited

mattip commented Nov 13, 2017

ahaldane commented Apr 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahaldane commented Apr 21, 2018

mattip commented Apr 21, 2018

ahaldane commented Apr 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahaldane commented Nov 10, 2017 •

edited

eric-wieser Nov 12, 2017 •

edited

eric-wieser Nov 12, 2017 •

edited

eric-wieser Nov 12, 2017 •

edited

eric-wieser Nov 12, 2017 •

edited

njsmith Nov 12, 2017 •

edited

eric-wieser Nov 12, 2017 •

edited

eric-wieser commented Nov 13, 2017 •

edited