WIP: indexing with broadcasting #1473

shoyer · 2017-07-10T01:49:32Z

Closes Best practice when the _Unsigned attribute is present in NetCDF files #1444, closes Wrong dimension referenced in boolean indexing with .loc #1436
Tests added / passed
Passes git diff master | flake8 --diff
Fully documented, including whats-new.rst for all changes and api.rst for new API

shoyer · 2017-07-10T02:08:20Z

xarray/core/variable.py

+            along self.dims.
+        """
+        if not utils.is_dict_like(key):
+            key = {self.dims[0]: key}


Note that if key is a tuple, it should be paired with multiple dimensions.

shoyer · 2017-07-10T02:26:21Z

xarray/core/variable.py

+                example_v = v
+            indexes[k] = v
+
+        # When all the keys are array or integer, slice


I think we actually need two totally different code paths for basic vs. advanced indexing:

If all indexers are integers/slices, we can use NumPy's "basic indexing". There's no need to futz with broadcasting. We can use exactly a simpler version of the existing logic, dropping _indexable_data. (We need this path because "basic indexing" is much faster than "advanced indexing" in NumPy, as it avoids copying data.)

If any indexers are arrays or Variable objects, we need to do "advanced indexing":

Normalize every (key, value) pair into xarray.Variable objects:

xarray.Variable value -> keep them unmodified

Integer values -> xarray.Variable((), value)

Slice values -> xarray.Variable((key,), np.arange(*value.indexes(size))) where size is the size of the dimension corresponding to key.

1D list/numpy arrays -> xarray.Variable((key,), value)

N-D arrays -> IndexError

Make all indexers broadcast compatible with a single call to _broadcast_compat_variables(). All variables are now guaranteed to have the same dims, so take dims from the first variable to label the result of indexing.

Create a tuple using data from each of these variables, and use it to index this variable's data.

Here's an alternative version (basically what I described above) that I think will work, at least if the variable's data is a NumPy array (we may need to jump through a few more hoops for dask arrays):

def _broadcast_indexes(self, key): key = self._item_key_to_tuple(key) # key is a tuple key = indexing.expanded_indexer(key, self.ndim) # key is a tuple of full size basic_indexing_types = integer_types + (slice,) if all(isinstance(k, basic_indexing_types) for k in key): return self._broadcast_indexes_basic(key) else: return self._broadcast_indexes_advanced(key) def _broadcast_indexes_basic(self, key): dims = tuple(dim for k, dim in zip(key, self.dims) if not isinstance(k, integer_types)) return dims, key def _broadcast_indexes_advanced(self, key): variables = [] for dim, value in zip(self.dims, key): if isinstance(value, slice): value = np.arange(*value.indices(self.sizes[dim]) # NOTE: this is close to but not quite correct, since we want to # handle tuples differently than as_variable and want a different # error message (not referencing tuples) variable = as_variable(value, name=dim) variables.append(variable) variables = _broadcast_compat_variables(*variables) dims = variables[0].dims # all variables have the same dims key = tuple(variable.data for variable in variables) return dims, key

@shoyer

We need this path because "basic indexing" is much faster than "advanced indexing" in NumPy, as it avoids copying data.

Thanks for the suggestion!
And also thanks for the reference implementation.
(What a simple logic!!)

Please let me make sure one thing.

What would be expected by the following?

v = Variable(['x', 'y'], [[0, 1, 2], [3, 4, 5]]) ind_x = Variable(['a'], [0, 1]) v.getitem2(dict(x=ind_x, y=[1, 0])))

It should be understood as y=Variable(['y'], [1, 0])?
I thought y=[0, 1] would be y=Variable(['a'], [1, 0])...

Yes, y=[0, 1] -> Variable(['y'], [1, 0]).

I think the result here would look like Variable(['a', 'y'], [[3, 4], [0, 1]]):

After making them broadcast compatible, the indexer variables have dimensions ['a', 'y'] and shapes (2, 1)/(1, 2).

For the x coordinate, each coordinate value is taken in order.

For the y coordinate, the first two coordinates are taken in reverse order.

The result is labeled by the dimensions of the indexers (i.e., ('a', 'y')).

fujiisoup · 2017-07-12T10:13:48Z

Thanks @shoyer
I updated _broadcast_indexes method based on your reference script.

As you pointed out, we may need better Error message in here.
I guess we should raise our own Exception class in as_variable and replace the message here?
Duplication of as_variable function only for better exception message sounds a bad idea.

fujiisoup · 2017-07-12T10:17:33Z

xarray/core/variable.py

+            try:  # TODO we need our own Exception.
+                variable = as_variable(value, name=dim)
+            except ValueError as e:
+                if "cannot set variable" in str(e):


My current implementation for this exception handling is rather bad.
I want to change this to something like

except DimensionMismatchError: raise IndexError('...')

I agree. Can you switch this for as_variable?

Inherit from ValueError for backwards compatibility. Something like MissingDimensionsError should be more broadly useful for cases like this where we can't safely guess a dimension name.

fujiisoup · 2017-07-16T08:19:11Z

I just realized that dask's indexing is limited, e.g. it does not support nd-array indexing.
I will try to make a work around this issue, but I am not very familiar with dask.
If anyone gives me any idea for this, it would be helpful.

shoyer · 2017-07-16T19:35:40Z

dask.array does support a limited form of fancy indexing via .vindex. I think we already use it in .isel_points(). It would certainly be better to use that and error in edge cases rather than silently converting to numpy arrays, which we never want to do.

shoyer · 2017-07-16T19:51:56Z

xarray/core/variable.py

+                else:
+                    raise e
+            if variable._isbool_type():  # boolean indexing case
+                variables.extend(list(variable.nonzero()))


I'm a little hesitate to allow multi-dimensional boolean indexers here. The problem with using extend() here is that method calls like a.isel(x=b) get mapped into something like a[b], so if b is multi-dimensional, the second dimension of b gets matched up with the second dimension of a in an unpredictable way. We would need some way to specify the mapping to multiple dimensions, something like a.isel((x,y)=b) (obviously not valid syntax).

Instead, I would error for boolean indexers with more than one dimension, and then convert with nonzero(), as you've done here.

Multi-dimensional boolean indexers are useful, but I think the main use-case is indexing with a single argument like x[y > 0], so we don't need fancy remapping between dimensions.

I agree. I add the sanity check for the boolean array.

shoyer · 2017-07-16T19:52:27Z

xarray/core/variable.py

+                                     "cannot be used for indexing.")
+                else:
+                    raise e
+            if variable._isbool_type():  # boolean indexing case


Can we make this just variable.dtype.kind == 'b'?

shoyer · 2017-07-16T20:01:56Z

xarray/core/variable.py

+
+        for dim, value in zip(self.dims, key):
+            if isinstance(value, slice):
+                value = np.arange(self.sizes[dim])[value]


Use the slice.indices method here, to construct the desired array without indexing:

In [14]: x = slice(0, 10, 2) In [15]: np.arange(*x.indices(5)) Out[15]: array([0, 2, 4])

shoyer · 2017-07-16T20:04:37Z

xarray/core/variable.py

+        elif isinstance(self._data, dask_array_type):
+            # TODO we should replace dask's native nonzero
+            # after https://github.com/dask/dask/issues/1076 is implemented.
+            nonzeros = np.nonzero(self.load()._data)


I would just do nonzeros = np.nonzero(self.data) instead of all these isinstance checks. You can leave the TODO, but I don't think we actually need it for indexing since currently we already load indexers into memory.

shoyer · 2017-07-16T20:09:21Z

xarray/core/variable.py

+            try:  # TODO we need our own Exception.
+                variable = as_variable(value, name=dim)
+            except ValueError as e:
+                if "cannot set variable" in str(e):


I agree. Can you switch this for as_variable?

Inherit from ValueError for backwards compatibility. Something like MissingDimensionsError should be more broadly useful for cases like this where we can't safely guess a dimension name.

shoyer · 2017-07-16T20:11:08Z

xarray/core/variable.py

@@ -412,15 +473,15 @@ def __setitem__(self, key, value):

        See __getitem__ for more details.
        """
-        key = self._item_key_to_tuple(key)
+        dims, index_tuple = self._broadcast_indexes(key)
        if isinstance(self._data, dask_array_type):
            raise TypeError("this variable's data is stored in a dask array, "


This is actually no longer true -- dask.array supports assignment in recent versions.

@shoyer
Currently, array[0, 1] = 1 gives NotImplementedError: Item assignment with <class 'tuple'> not supported

I couldn't find its API. This one?

shoyer · 2017-07-16T20:14:10Z

xarray/core/variable.py

-        data = orthogonally_indexable(self._data)
-        data[key] = value
+        data = broadcasted_indexable(self._data)
+        data[index_tuple] = value


value should be broadcast against the indexing key if possible.

If it's an xarray.Variable, I think we can just call value.set_dims(dims) on it. If it's a NumPy/Dask array, I think we can safely ignore it and let NumPy/Dask handle broadcasting.

shoyer · 2017-07-16T20:17:14Z

xarray/core/indexing.py

    """
    def __init__(self, array):
        self.array = array

    def __getitem__(self, key):
-        key = expanded_indexer(key, self.ndim)
-        if any(not isinstance(k, integer_types + (slice,)) for k in key):
+        """ key: tuple of Variable, slice, integer """


I think it should only be a NumPy or dask array here, not a Variable.

shoyer · 2017-07-16T20:18:30Z

xarray/core/indexing.py

-        if any(not isinstance(k, integer_types + (slice,)) for k in key):
+        """ key: tuple of Variable, slice, integer """
+        # basic or orthogonal indexing
+        if all(isinstance(k, (integer_types, slice)) or k.squeeze().ndim <= 1


For simple cases where everything is an integers or slice, we want to just use a single indexing call like self.array[key] rather than this loop.

In the hard case when some arguments are arrays, we should try self.array.vindex[key]. If it doesn't work in some cases, we can either add work-arounds or try to fix it upstream in dask.

After looking at dask.array in a little more detail, I think we need to keep a work-around for "orthogonal" indexing in dask. It looks like vindex only works when each indexer is 1D.

See fujiisoup#1

Refactor DaskArrayAdapter

shoyer · 2017-07-17T02:58:57Z

Let's not worry about supporting every indexing type with dask. I think that with my patch we can do everything we currently do. We'll want vindex support eventually as well so we can remove isel_points(), but that can come later.

I think we'll also want to make an "vectorized to orthogonal" indexing adapter that we can use netCDF4.Variable.

…_getitem__

…ice]`.

# Conflicts: # xarray/core/variable.py

fujiisoup · 2017-07-17T12:49:46Z

@shoyer Thanks for your help.

Let's not worry about supporting every indexing type with dask.

Yes. Thanks to your patch, dask-based variable is now indexed fine.

Some replies to your comments to the outdated codes.

multidimensional boolean indexer
Agree. I added a sanity check and raise IndexError in case of multi-dimensional boolean array.
indexer type in DasokIndexingAdapter
Because I changed how indexing.broadcasted_indexable (formally indexing.orthogonally_indexable) is called, indexers passed to DaskIndexingAdapter are already broadcasted to Variables (in case of _broadcast_indexes_advanced).

I will try to fit the other array wrappers, LazilyIndexedArray, CopyOnWriteArray, MemoryCachedArray to the broadcasted indexers, the tests of which currently fail.
(Maybe I will need another help.)

fujiisoup · 2017-09-22T15:32:58Z

I think it would be better to update reindex method in another PR, as this PR is already large.
So I ported this suggestion to #1553.

jhamman · 2017-09-25T15:01:14Z

@shoyer and @fujiisoup - is this ready for a final review?

fujiisoup · 2017-09-25T15:35:03Z

I think it's ready.
I appreciate any further comments.

jhamman

Wow! This PR is a behemoth. I think I made it through the whole thing. I'm really excited about all these changes so nice work.

jhamman · 2017-09-25T16:33:01Z

doc/indexing.rst

-Note that using ``sel`` it is not possible to mix a dimension
-indexer with level indexers for that dimension
-(e.g., ``mda.sel(x={'one': 'a'}, two=0)`` will raise a ``ValueError``).
+In briefly, similar to how NumPy's `advanced indexing`_ works, vectorized


~~In briefly~~

jhamman · 2017-09-25T16:34:28Z

doc/indexing.rst

-dimensions or use the ellipsis in the ``loc`` specifier, e.g. in the example
-above, ``mda.loc[{'one': 'a', 'two': 0}, :]`` or ``mda.loc[('a', 0), ...]``.
+    ind = xr.DataArray([['a', 'b'], ['b', 'a']], dims=['a', 'b'])
+    da.loc[:, ind]  # same to da.sel(y=ind)


jhamman · 2017-09-25T16:35:42Z

doc/indexing.rst


-Multi-dimensional indexing
--------------------------
+and also for ``Dataset``


These methods may also be applied to Dataset objects

jhamman · 2017-09-25T16:39:20Z

doc/indexing.rst

-using numpy's broadcasting rules to vectorize indexers. This means you can do
-indexing like this, which would require slightly more awkward syntax with
-numpy arrays:
+As like numpy ndarray, value assignment sometimes works differently from what one may expect.


Like numpy.ndarray, ...

jhamman · 2017-09-25T16:40:33Z

doc/indexing.rst

+
+
+.. note::
+  Dask backend does not yet support value assignment


Dask arrays do not support value assignment

jhamman · 2017-09-25T22:26:43Z

xarray/core/variable.py

@@ -99,7 +111,7 @@ def as_variable(obj, name=None):
    if name is not None and name in obj.dims:
        # convert the Variable into an Index
        if obj.ndim != 1:
-            raise ValueError(
+            raise MissingDimensionsError(
                '%r has more than 1-dimension and the same name as one of its '
                'dimensions %r. xarray disallows such variables because they '
                'conflict with the coordinates used to label dimensions.'


this may be missing % (name, data.ndim)

jhamman · 2017-09-25T22:38:31Z

xarray/core/variable.py

+        # after https://github.com/dask/dask/issues/1076 is implemented.
+        nonzeros = np.nonzero(self.data)
+        return tuple(Variable((dim), nz) for nz, dim
+                     in zip(nonzeros, self.dims))


So this is in dask now. Can we add a conditional import of the dask version here?

Side note, I think a nonzero method just like this would fit in xarray's public API (not part of this PR).

jhamman · 2017-09-25T22:42:55Z

xarray/core/variable.py

+        data = self._indexable_data[index_tuple]
+        if new_order:
+            data = np.moveaxis(data, range(len(new_order)), new_order)
+        assert getattr(data, 'ndim', 0) == len(dims), (data.ndim, len(dims))


Can we raise a ValueError here instead and provide more informative error message?

Actually, I don't think of the case this assertion fails.
Maybe I will remove this line.

jhamman · 2017-09-25T22:51:48Z

xarray/tests/test_dataset.py

+            assert all(["Indexer has dimensions " not in
+                        str(w.message) for w in ws])
+            warnings.warn('dummy', FutureWarning, stacklevel=3)
+


2 things here:

I'm not sure why we need the dummy warning if it should not warn. Can we assert that a warning was not raised?

move the ind = ... line out of the context manager so it is clear which line should be raising a warning

I do not know a good way to make sure that the code does NOT warn.
If the dummy warning line is removed, the test fails because the code does not warn.

Does anyone know a better workaround?

I think this SO answer should work.

ind = xr.DataArray([0.0, 1.0], dims=['dim2'], name='ind') with pytest.warns(None) as ws: # Should not warn data.reindex(dim2=ind) assert len(ws) == 0

Thanks. It now works without dummy warning.

jhamman · 2017-09-25T22:53:19Z

xarray/tests/test_dataset.py

+        if pd.__version__ >= '0.17':
+            with self.assertRaises(KeyError):
+                data.sel_points(x=[2.5], y=[2.0], method='pad', tolerance=1e-3)
+


move to its own test and use pytest.importorskip('pandas', minversion='0.17') in place of your if statement

shoyer

I still need to look carefully at the logic for coordinate conflicts.

shoyer · 2017-09-26T01:56:19Z

xarray/core/nputils.py

+        if current != previous + 1:
+            return False
+        previous = current
+    return True


Actually, all() is defined in the right way for length 0 lists. So this can just be:

def _is_contiguous(positions): return (np.diff(positions) == 1).all()

This is arguably an improvement for readability (at least for the numpy familiar devs who work on xarray) but there's no advantage to using this because positions only has one element per indexer, so it almost always a list of only a few items.

fujiisoup · 2017-10-04T01:20:13Z

@jhamman Thanks for the review (and sorry for my late reply).
I made some modifications.

@shoyer
Do you have further comments about coordinate confliction?

Limitations of the current implementation are

Coordinate confliction and attachment related to reindex is still off. I think it should go with another PR.
I could not solve the 2nd issue of this comment.
In your exampe,

mda.sel(x=xr.DataArray(mda.indexes['x'][:3], dims='x'))

works as expected, but

mda.sel(x=xr.DataArray(mda.indexes['x'][:3], dims='z'))

will attach coordinate z.

shoyer · 2017-10-09T20:42:53Z

@fujiisoup Thanks again for all your hard work on this and for my slow response. I've made another PR with tweaks to your logic for conflicting coordinates: fujiisoup#5

Mostly, my PR is about simplifying the logic by removing the special case work arounds you added that check object identity (things like this_arr is self._variables[k] and v.variable is cv.variable). My concern is that nothing else in xarray relies on these types of identity checks, so adding these additional rules will make the logic harder to understand and rely on programmatically. If we try to make behavior "intuitive" for 80% of use-cases, it only makes the remaining 20% more baffling and error prone. This is the similar to the problem with dimension re-ordering and NumPy's mixed integer/slice indexing. So I would rather these leave out for now at the cost of making the API slightly more cumbersome. As I think we discussed previously, it is easier to relax error conditions in the future than to add new errors.

shoyer · 2017-10-09T20:46:13Z

I will take a look at the multi-index issues. I suspect that many of these will be hard to resolve until we complete the refactor making indexes an explicit part the data model (#1603). It is really tricky to make things work reliably when MultiIndex levels are supposed to work like coordinates but use an entirely different mechanism. I would be OK with adding a warning that there are still a few unresolved edge cases involving MultiIndex.

Simplify rules for indexing conflicts

fujiisoup · 2017-10-09T23:45:55Z

@shoyer, thanks for your review.

If we try to make behavior "intuitive" for 80% of use-cases, it only makes the remaining 20% more baffling and error prone.

OK. It makes sense also for me. Merging your PR.

I would be OK with adding a warning that there are still a few unresolved edge cases involving MultiIndex.

Actually, the vectorized label-indexing currently does not work almost entirely with MultiIndex.
I think of the following cases where appropriate error messages are required,

In [1]: import xarray as xr
   ...: import pandas as pd
   ...: 
   ...: midx = pd.MultiIndex.from_tuples(
   ...:          [(1, 'a'), (2, 'b'), (3, 'c')],
   ...:          names=['x0', 'x1'])
   ...: da = xr.DataArray([0, 1, 2], dims=['x'], 
   ...:                   coords={'x': midx})
   ...: da
   ...: 
Out[1]: 
<xarray.DataArray (x: 3)>
array([0, 1, 2])
Coordinates:
  * x        (x) MultiIndex
  - x0       (x) int64 1 2 3
  - x1       (x) object 'a' 'b' 'c'

da.sel(x=[(1, 'a'), (2, 'b')])
da.sel(x0='a')

works as expected,

da.sel(x0=[1, 2])

fail without appropriate error messages

da.sel(x=xr.DataArray([np.array(midx[:2]), np.array(midx[-2:])], dims=['y', 'z']))

destructs the MultiIndex structure silently.

I will add better Exceptions later today.

shoyer · 2017-10-19T01:16:48Z

I think this is ready to go in. @jhamman @fujiisoup any reason to wait?

fujiisoup · 2017-10-19T05:40:57Z

I'm happy with this :)

jhamman · 2017-10-19T16:38:35Z

LGTM.

shoyer · 2017-10-19T16:52:44Z

@fujiisoup Can you open a new pull request with this branch? I'd like to give you credit on GitHub for this (since you did most of the work), but I think if I merge this with "Squash and Merge" everything will get credited to me.

You can also try doing your own rebase to clean-up history into fewer commits if you like (or I could "squash and merge" locally in git), but I think the new PR would do a better job of preserving history anyone who wants to look at this later.

shoyer · 2017-10-19T16:53:28Z

I closed this intentionally since I think there is a good chance GitHub won't let you open a new PR otherwise.

fujiisoup added 2 commits July 8, 2017 00:01

Implemented _broadcast_indexes in Variable.py

105bd64

Diagonal indexing for Variable.

23b4fe0

shoyer mentioned this pull request Jul 10, 2017

Indexing with alignment and broadcasting #974

Closed

shoyer commented Jul 10, 2017

View reviewed changes

update _broadcast_indexes. update tests.

726ba5d

fujiisoup reviewed Jul 12, 2017

View reviewed changes

jhamman added the topic-indexing label Jul 13, 2017

fujiisoup added 3 commits July 16, 2017 16:01

Support basic boolean indexing.

df7011f

tests for dask-based Variable

f9232cb

Explicitly mark xfail flags

17b6465

orthogonal indexing for dask.

33c51d3

shoyer commented Jul 16, 2017

View reviewed changes

shoyer and others added 2 commits July 16, 2017 15:48

Refactor DaskArrayAdapter

03a336f

Merge pull request #1 from shoyer/indexing_broadcasting

d5af395

Refactor DaskArrayAdapter

fujiisoup added 3 commits July 17, 2017 16:35

Added MissingDimensionsError. Improve DaskIndexingAdapter, Variable._…

866de91

…_getitem__

use np.arange(*slice.indices(size)) rather than `np.arange(size)[sl…

08e7444

…ice]`.

Merge branch 'master' into indexing_broadcasting

84afc98

# Conflicts: # xarray/core/variable.py

fujiisoup added 8 commits July 20, 2017 21:32

Add orthogonalize_indexers

7b33269

A bug fix.

50ea56e

Working with LazilyIndexedArray

bac0089

Fix in LazilyIndexedArray.

1206c28

add @requires_dask in test_variable

c2747be

rename orthogonalize_indexers -> unbroadcast_indexers

0671f39

Wrap LazilyIndexedArray so that it accepts broadcasted-indexers

ffccff1

small rename

becf539

jhamman requested changes Sep 25, 2017

View reviewed changes

shoyer commented Sep 26, 2017

View reviewed changes

fujiisoup added 5 commits September 27, 2017 19:30

Merge remote-tracking branch 'pydata/master' into indexing_broadcasting

9523039

Fixes based on jhamman's comments.

dc60348

Improve test for warning.

8a62ad9

Merge branch 'master' into indexing_broadcasting

6b96960

Remove unused assert sentence.

cb84154

Simplify rules for indexing conflicts

9726531

Merge pull request #5 from shoyer/indexing_broadcasting

caa79fe

Simplify rules for indexing conflicts

Better error-message for multiindex vectorized-selection.

170abc5

shoyer mentioned this pull request Oct 17, 2017

DataArray.argsort should be deleted #1635

Open

jhamman approved these changes Oct 19, 2017

View reviewed changes

jhamman closed this Oct 19, 2017

jhamman reopened this Oct 19, 2017

shoyer closed this Oct 19, 2017

fujiisoup mentioned this pull request Oct 19, 2017

indexing with broadcasting #1639

Merged

4 tasks

Zac-HD mentioned this pull request Oct 23, 2017

Using uint64 for Dataset indexing gives ValueError #1405

Closed

fujiisoup deleted the indexing_broadcasting branch November 4, 2017 08:29

WIP: indexing with broadcasting #1473

WIP: indexing with broadcasting #1473

Conversation

shoyer commented Jul 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shoyer Jul 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fujiisoup commented Jul 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fujiisoup commented Jul 16, 2017

shoyer commented Jul 16, 2017

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

shoyer Jul 16, 2017 • 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

Choose a reason for hiding this comment

shoyer commented Jul 17, 2017

fujiisoup commented Jul 17, 2017 • edited Loading

fujiisoup commented Sep 22, 2017

jhamman commented Sep 25, 2017

fujiisoup commented Sep 25, 2017 • edited Loading

jhamman left a 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

shoyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fujiisoup commented Oct 4, 2017

shoyer commented Oct 9, 2017

shoyer commented Oct 9, 2017

fujiisoup commented Oct 9, 2017 • edited Loading

shoyer commented Oct 19, 2017

fujiisoup commented Oct 19, 2017

jhamman commented Oct 19, 2017

shoyer commented Oct 19, 2017

shoyer commented Oct 19, 2017

shoyer commented Jul 10, 2017 •

edited

Loading

shoyer Jul 10, 2017 •

edited

Loading

shoyer Jul 16, 2017 •

edited

Loading

fujiisoup commented Jul 17, 2017 •

edited

Loading

fujiisoup commented Sep 25, 2017 •

edited

Loading

fujiisoup commented Oct 9, 2017 •

edited

Loading