home / github

Menu
  • GraphQL API
  • Search all tables

issue_comments

Table actions
  • GraphQL API for issue_comments

11 rows where author_association = "MEMBER", issue = 129286220 and user = 1217238 sorted by updated_at descending

✎ View and edit SQL

This data as json, CSV (advanced)

Suggested facets: created_at (date), updated_at (date)

user 1

  • shoyer · 11 ✖

issue 1

  • Add tensordot to dataarray class also add its test to test_dataarray · 11 ✖

author_association 1

  • MEMBER · 11 ✖
id html_url issue_url node_id user created_at updated_at ▲ author_association body reactions performed_via_github_app issue
192110026 https://github.com/pydata/xarray/pull/731#issuecomment-192110026 https://api.github.com/repos/pydata/xarray/issues/731 MDEyOklzc3VlQ29tbWVudDE5MjExMDAyNg== shoyer 1217238 2016-03-04T05:23:00Z 2016-03-04T05:23:00Z MEMBER

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?

{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
  Add tensordot to dataarray class also add its test to test_dataarray 129286220
189529053 https://github.com/pydata/xarray/pull/731#issuecomment-189529053 https://api.github.com/repos/pydata/xarray/issues/731 MDEyOklzc3VlQ29tbWVudDE4OTUyOTA1Mw== shoyer 1217238 2016-02-26T23:45:42Z 2016-02-26T23:45:42Z MEMBER

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.

{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
  Add tensordot to dataarray class also add its test to test_dataarray 129286220
184041128 https://github.com/pydata/xarray/pull/731#issuecomment-184041128 https://api.github.com/repos/pydata/xarray/issues/731 MDEyOklzc3VlQ29tbWVudDE4NDA0MTEyOA== shoyer 1217238 2016-02-15T03:01:22Z 2016-02-15T03:01:57Z MEMBER

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.

{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
  Add tensordot to dataarray class also add its test to test_dataarray 129286220
178705897 https://github.com/pydata/xarray/pull/731#issuecomment-178705897 https://api.github.com/repos/pydata/xarray/issues/731 MDEyOklzc3VlQ29tbWVudDE3ODcwNTg5Nw== shoyer 1217238 2016-02-02T17:36:11Z 2016-02-02T17:36:11Z MEMBER

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

{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
  Add tensordot to dataarray class also add its test to test_dataarray 129286220
178686155 https://github.com/pydata/xarray/pull/731#issuecomment-178686155 https://api.github.com/repos/pydata/xarray/issues/731 MDEyOklzc3VlQ29tbWVudDE3ODY4NjE1NQ== shoyer 1217238 2016-02-02T16:58:02Z 2016-02-02T16:58:02Z MEMBER

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

{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
  Add tensordot to dataarray class also add its test to test_dataarray 129286220
178299486 https://github.com/pydata/xarray/pull/731#issuecomment-178299486 https://api.github.com/repos/pydata/xarray/issues/731 MDEyOklzc3VlQ29tbWVudDE3ODI5OTQ4Ng== shoyer 1217238 2016-02-02T01:38:37Z 2016-02-02T01:38:37Z MEMBER

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

{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
  Add tensordot to dataarray class also add its test to test_dataarray 129286220
178299410 https://github.com/pydata/xarray/pull/731#issuecomment-178299410 https://api.github.com/repos/pydata/xarray/issues/731 MDEyOklzc3VlQ29tbWVudDE3ODI5OTQxMA== shoyer 1217238 2016-02-02T01:38:10Z 2016-02-02T01:38:10Z MEMBER

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

{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
  Add tensordot to dataarray class also add its test to test_dataarray 129286220
176869693 https://github.com/pydata/xarray/pull/731#issuecomment-176869693 https://api.github.com/repos/pydata/xarray/issues/731 MDEyOklzc3VlQ29tbWVudDE3Njg2OTY5Mw== shoyer 1217238 2016-01-29T17:20:50Z 2016-01-29T17:20:50Z MEMBER

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).

{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
  Add tensordot to dataarray class also add its test to test_dataarray 129286220
176555551 https://github.com/pydata/xarray/pull/731#issuecomment-176555551 https://api.github.com/repos/pydata/xarray/issues/731 MDEyOklzc3VlQ29tbWVudDE3NjU1NTU1MQ== shoyer 1217238 2016-01-29T03:40:32Z 2016-01-29T03:40:32Z MEMBER

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

{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
  Add tensordot to dataarray class also add its test to test_dataarray 129286220
176553874 https://github.com/pydata/xarray/pull/731#issuecomment-176553874 https://api.github.com/repos/pydata/xarray/issues/731 MDEyOklzc3VlQ29tbWVudDE3NjU1Mzg3NA== shoyer 1217238 2016-01-29T03:31:51Z 2016-01-29T03:31:51Z MEMBER

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.

{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
  Add tensordot to dataarray class also add its test to test_dataarray 129286220
176374932 https://github.com/pydata/xarray/pull/731#issuecomment-176374932 https://api.github.com/repos/pydata/xarray/issues/731 MDEyOklzc3VlQ29tbWVudDE3NjM3NDkzMg== shoyer 1217238 2016-01-28T19:54:02Z 2016-01-28T19:54:02Z MEMBER

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.

{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
  Add tensordot to dataarray class also add its test to test_dataarray 129286220

Advanced export

JSON shape: default, array, newline-delimited, object

CSV options:

CREATE TABLE [issue_comments] (
   [html_url] TEXT,
   [issue_url] TEXT,
   [id] INTEGER PRIMARY KEY,
   [node_id] TEXT,
   [user] INTEGER REFERENCES [users]([id]),
   [created_at] TEXT,
   [updated_at] TEXT,
   [author_association] TEXT,
   [body] TEXT,
   [reactions] TEXT,
   [performed_via_github_app] TEXT,
   [issue] INTEGER REFERENCES [issues]([id])
);
CREATE INDEX [idx_issue_comments_issue]
    ON [issue_comments] ([issue]);
CREATE INDEX [idx_issue_comments_user]
    ON [issue_comments] ([user]);
Powered by Datasette · Queries took 233.096ms · About: xarray-datasette