Add tensordot to dataarray class also add its test to test_dataarray #731

deanpospisil · 2016-01-27T22:10:15Z

Resolving issue #723

deanpospisil · 2016-01-27T23:07:37Z

It seems in both unsuccesful cases it was trying to import dask and failed, I can't quite figure out what I could have changed to prevent dask from importing:

>       da = da.chunk()
xarray/test/test_dataarray.py:1618: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
xarray/core/dataarray.py:569: in chunk
    ds = self._to_temp_dataset().chunk(chunks)

        try:
            from dask.base import tokenize
        except ImportError:
>           import dask  # raise the usual error if dask is entirely missing
E           ImportError: No module named 'dask'

shoyer · 2016-01-28T19:48:35Z

xarray/core/dataarray.py

@@ -1369,6 +1369,27 @@ def real(self):
    @property
    def imag(self):
        return self._replace(self.variable.imag)
+
+    def tensordot( self, b, dims):


I would call the second argument "other" rather than b

shoyer · 2016-01-28T19:54:02Z

This also needs docs, minimally including a note in what's new (for v0.7.1), a docstring and a reference in the API docs.

shoyer · 2016-01-28T19:54:45Z

xarray/core/dataarray.py

+    def tensordot( self, b, dims):
+        a = self
+        if not (isinstance(a, DataArray) and isinstance(b, DataArray)):
+            raise ValueError


This error should be more descriptive -- and probably a TypeError instead

shoyer · 2016-01-29T03:31:51Z

I'll just note that one other logical way to do this would be to implement tensordot as an xray.Variable method, and implement DataArray.tensordot in terms of it. This is a little closer to how we've written existing methods in xarray. It would probably help eventually for if/when we implement this on Dataset (by consolidating code) but it's not really necessary now.

shoyer · 2016-01-29T03:40:32Z

The test is failing because we don't have dask installed in all builds -- because dask is an optional dependency, and we want to make things still work when you don't have it installed.

We do have tests setup to skip if dask is not installed, by using the @requires_dask decorator. In this case, I would recommend moving the dask-specific tests to one of the existing test classes we've set up in test_dask.py. Take a look at this test case for an example of what this could look like:
https://github.com/pydata/xarray/blob/v0.7.0/xarray/test/test_dask.py#L264-L267

deanpospisil · 2016-01-29T04:44:25Z

I've implemented all your comments, except:

In the dask test I'm having a little trouble with:
self.assertLazyAndAllClose(eager, lazy)
working with the DataArrays I make. Does it only take transformations of:

self.eager_array 
self.lazy_array

2 Your comment:
Use the genetic type(self) instead
Which I didn't understand.

3 Need to figure out how to edit the docs.

And I was thinking:
The default (when no dims arg is given) should be to tensordot over all shared dims between DataArrays.
And it should prevent you from not summing over all shared dims, since that will return a DataArray with repeated labels. I think it would be rare for people to want to tensor dot over different dimensions.

Thanks for all your help getting me going!

shoyer · 2016-01-29T17:20:50Z

In the dask test I'm having a little trouble with: self.assertLazyAndAllClose(eager, lazy) working with the DataArrays I make. Does it only take transformations of:
self.eager_array
self.lazy_array

Nope, it can handle any eager (numpy) and lazy (dask) xarray objects. Something like this should work:

# depending on exactly what syntax we support
eager = self.eager_array.tensordot(self.eager_array[0])
lazy = self.lazy_array.tensordot(self.lazy_array[0]))
self.assertLazyAndAllClose(eager, lazy)

The default (when no dims arg is given) should be to tensordot over all shared dims between DataArrays.
And it should prevent you from not summing over all shared dims, since that will return a DataArray with repeated labels. I think it would be rare for people to want to tensor dot over different dimensions.

This is a good point! Arrays with redundant dimensions are not very useful.

The sane thing to do is to broadcast over dimensions that aren't being summed einsum style (e.g., like i in einsum('ij,ij->i', x, y)). This is similar to the way that @/np.matmul works.

That said, this is difficult to implement with numpy's dot/tensordot, so perhaps it's better to simply error or omit the dims argument entirely for now. Eventually, we might be able to do this using @/np.matmul (only in numpy 1.10 and newer).

I also wonder if perhaps we should rename this from tensordot to simply dot. I don't think we would want to use dot for anything else, and it might also be nice to support @ syntax as an alias for this in Python 3 (again, at some later point).

deanpospisil · 2016-01-29T20:29:15Z

That said, this is difficult to implement with numpy's dot/tensordot, so perhaps it's better to simply error or omit the dims argument entirely for now. Eventually, we might be able to do this using @/np.matmul (only in numpy 1.10 and newer).

Yeah I'm going to go for omitting the dims entirely for now, I think it makes the function easy to call, and really covers what I imagine most people would use the function for, getting the relationship between two DataArrays along their common dimensions. Thats all I'm going to use it for.

I also wonder if perhaps we should rename this from tensordot to simply dot. I don't think we would want to use dot for anything else, and it might also be nice to support @ syntax as an alias for this in Python 3 (again, at some later point).

Yeah that makes sense. tensordot in xarray seems like it would be the function that allows more flexibility in how a sum product is applied, dot can be thought of as a reduced version of tensordot taking no dims argument.

shoyer · 2016-02-02T01:34:20Z

xarray/core/dataarray.py

+
+    def dot( self, other):
+        """Perform sum product of two DataArrays along their shared dims. 
+        Equivalent to taking taking tensor dot over all shared dims'''


Should end in ., not '''

Also, add a line break to separate it from the first line.

shoyer · 2016-02-02T01:38:10Z

Take a look at PEP8 guidelines on spaces -- you are inserting a few extras around your parentheses :)

shoyer · 2016-02-02T01:38:37Z

a few more comments forthcoming, generally looks very good, though

deanpospisil · 2016-02-02T05:12:33Z

Thanks! Good comments, should help with my next pull request. : )

shoyer · 2016-02-02T16:44:09Z

xarray/core/dataarray.py

+            raise TypeError('dot only operates on DataArrays.')
+
+        #sum over the common dims
+        dims = list(set(s.dims) & set(other.dims) )


no need to turn these into a list -- a set is a perfectly valid sequence for use below, as well

shoyer · 2016-02-02T16:58:02Z

I recommend running a tool like PEP8 to check the style here: https://pypi.python.org/pypi/pep8

shoyer · 2016-02-02T16:58:41Z

doc/whats-new.rst

+
+Enhancements
+~~~~~~~~~~~~
+-xarray version of np.dot :py:meth:`~DataArray.dot`. Takes the sum product over the shared dimensions of two DataArrays. Can be useful for measuring correlation over common dimensions of two DataArrays.


I don't think you need "Can be useful for..."

Dot products are pretty broadly useful in science :)

shoyer · 2016-02-02T17:36:11Z

You'll need to squash these into one commit (git rebase -i) and then rebase on master to fix what's new. See here for more details: http://pandas.pydata.org/pandas-docs/stable/contributing.html#combining-commits

deanpospisil · 2016-02-14T01:15:49Z

Alright I think I got rebase to work, apologies for the delay.
There were some comments I wasn't sure about how to integrate, I asked about them, but I figured the best way to keep moving forward was to push my best guess at what you meant. So we might need to go through one more round of comments. Thanks for being patient!

shoyer · 2016-02-15T02:27:05Z

xarray/core/dataarray.py

@@ -1369,6 +1369,83 @@ def real(self):
    @property
    def imag(self):
        return self._replace(self.variable.imag)
+


You still have the tensordot method defined -- you should delete it now that it's replaced by dot :)

shoyer · 2016-02-15T03:01:22Z

I'm pleased to see how this is coming along. No need to apologize for the delay -- this stuff takes time and practice to figure out :).

I'd like to see more test cases so we can be confident this works properly. At the very least, we should test a dot product with multiple dimensions in common. Other edge cases worth checking:

all dimensions in common (e.g., dot product of an array with itself)
no dimensions in common (this should give a sensible error message -- does it?)
dot product with a scalar DataArray (should also error)
dot product with a Dataset (verify that this raises NotImplementedError using self.assertRaises or self.assertRaisesRegexp)
dot product with a numpy array (this should also error)

In generally, it's hard to have too many tests. One indication that you may not have enough tests comes from the "coveralls" status check, which you can find if you click on "Show all checks" at the bottom of the PR. Ideally, each PR should only increase, not decrease code coverage -- the idea is that unit tests should run over every possible code pathway.

deanpospisil · 2016-02-26T20:44:05Z

Ready for review.

jhamman · 2016-02-26T20:47:49Z

xarray/core/dataarray.py

+            raise NotImplementedError('dot products are not yet supported '
+                                      'with Dataset objects.')
+        if not isinstance(other, DataArray):
+            raise TypeError('dot only operates on DataArrays.')


I think it would be fine to remove lines 1413:1415, and change 1417 to something like:

if not isinstance(other, DataArray): raise TypeError('dot only operates on DataArrays, got {}'.format(type(other))

Yeah, could go either way here. I like raising NotImplementedError because it makes it more obvious to users that this behavior might change in the future. Besides, if we get lucky a user will notice the error and then implement the missing functionality themselves ;).

shoyer · 2016-02-26T23:45:42Z

I made two very minor suggestions for improvement inline, but I think the code here looks ready!

The only thing we need at now is hook up the documentation! Please add dot to api.rst and a release note in whats-new.rst.

…add tensordot to ops

deanpospisil · 2016-03-04T05:09:54Z

Alright, I think it's ready!

Add tensordot to dataarray class also add its test to test_dataarray

shoyer · 2016-03-04T05:23:00Z

OK, let's get this in. @deanpospisil thanks for your contribution!

@jhamman I'm thinking it's probably worth issuing a 0.7.2 release shortly so we can get dot and rolling out into the wild?

jhamman · 2016-03-04T05:39:13Z

+1 on releasing 0.7.2. If we can get #782 in there too, that would be great.

shoyer reviewed Jan 28, 2016
View reviewed changes

shoyer reviewed Feb 2, 2016
View reviewed changes

deanpospisil force-pushed the feature/tensordot_issue723 branch 2 times, most recently from 685a050 to cc0c24a Compare February 14, 2016 01:12

shoyer reviewed Feb 15, 2016
View reviewed changes

deanpospisil force-pushed the feature/tensordot_issue723 branch 4 times, most recently from 68b711b to 6539fcd Compare February 23, 2016 07:09

jhamman reviewed Feb 26, 2016
View reviewed changes

add dot to DataArray, add tests for dask and DataArray, update docs, …

ebee516

…add tensordot to ops

deanpospisil force-pushed the feature/tensordot_issue723 branch from 6539fcd to ebee516 Compare March 3, 2016 18:28

shoyer added a commit that referenced this pull request Mar 4, 2016

Merge pull request #731 from deanpospisil/feature/tensordot_issue723

4dba0f9

Add tensordot to dataarray class also add its test to test_dataarray

shoyer merged commit 4dba0f9 into pydata:master Mar 4, 2016

deanpospisil deleted the feature/tensordot_issue723 branch March 4, 2016 06:39

shoyer mentioned this pull request Mar 5, 2016

Implement tensordot for xarray with dask support #723

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tensordot to dataarray class also add its test to test_dataarray #731

Add tensordot to dataarray class also add its test to test_dataarray #731

deanpospisil commented Jan 27, 2016

deanpospisil commented Jan 27, 2016

shoyer Jan 28, 2016

shoyer commented Jan 28, 2016

shoyer Jan 28, 2016

shoyer commented Jan 29, 2016

shoyer commented Jan 29, 2016

deanpospisil commented Jan 29, 2016

shoyer commented Jan 29, 2016

deanpospisil commented Jan 29, 2016

shoyer Feb 2, 2016

shoyer commented Feb 2, 2016

shoyer commented Feb 2, 2016

deanpospisil commented Feb 2, 2016

shoyer Feb 2, 2016

shoyer commented Feb 2, 2016

shoyer Feb 2, 2016

shoyer commented Feb 2, 2016

deanpospisil commented Feb 14, 2016

shoyer Feb 15, 2016

shoyer commented Feb 15, 2016

deanpospisil commented Feb 26, 2016

jhamman Feb 26, 2016

shoyer Feb 26, 2016

shoyer commented Feb 26, 2016

deanpospisil commented Mar 4, 2016

shoyer commented Mar 4, 2016

jhamman commented Mar 4, 2016

Add tensordot to dataarray class also add its test to test_dataarray #731

Add tensordot to dataarray class also add its test to test_dataarray #731

Conversation

deanpospisil commented Jan 27, 2016

deanpospisil commented Jan 27, 2016

Choose a reason for hiding this comment

shoyer commented Jan 28, 2016

Choose a reason for hiding this comment

shoyer commented Jan 29, 2016

shoyer commented Jan 29, 2016

deanpospisil commented Jan 29, 2016

shoyer commented Jan 29, 2016

deanpospisil commented Jan 29, 2016

Choose a reason for hiding this comment

shoyer commented Feb 2, 2016

shoyer commented Feb 2, 2016

deanpospisil commented Feb 2, 2016

Choose a reason for hiding this comment

shoyer commented Feb 2, 2016

Choose a reason for hiding this comment

shoyer commented Feb 2, 2016

deanpospisil commented Feb 14, 2016

Choose a reason for hiding this comment

shoyer commented Feb 15, 2016

deanpospisil commented Feb 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shoyer commented Feb 26, 2016

deanpospisil commented Mar 4, 2016

shoyer commented Mar 4, 2016

jhamman commented Mar 4, 2016