issue_comments

Table actions

9 rows where issue = 211323643 sorted by updated_at descending

✎ View and edit SQL

This data as json, CSV (advanced)

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

issue 1

  • Added a support for Dataset.rolling. · 9 ✖

author_association 1

id html_url issue_url node_id user created_at updated_at ▲ author_association body reactions performed_via_github_app issue
290604141 https://github.com/pydata/xarray/pull/1289#issuecomment-290604141 https://api.github.com/repos/pydata/xarray/issues/1289 MDEyOklzc3VlQ29tbWVudDI5MDYwNDE0MQ== shoyer 1217238 2017-03-31T03:10:53Z 2017-03-31T03:10:53Z MEMBER

This is great -- thanks @fujiisoup !

 
{
    "total_count": 1,
    "+1": 1,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Added a support for Dataset.rolling. 211323643
289036993 https://github.com/pydata/xarray/pull/1289#issuecomment-289036993 https://api.github.com/repos/pydata/xarray/issues/1289 MDEyOklzc3VlQ29tbWVudDI4OTAzNjk5Mw== fujiisoup 6815844 2017-03-24T14:26:16Z 2017-03-24T14:26:16Z MEMBER

@shoyer Thank you for the reply. I did not yet think of smarter solution. I hope the current implementation is acceptable. If I come up with cleaner script, I will send another PR.

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Added a support for Dataset.rolling. 211323643
288866331 https://github.com/pydata/xarray/pull/1289#issuecomment-288866331 https://api.github.com/repos/pydata/xarray/issues/1289 MDEyOklzc3VlQ29tbWVudDI4ODg2NjMzMQ== max-sixty 5635139 2017-03-23T21:30:50Z 2017-03-23T21:30:50Z MEMBER

I'll let @MaximilianR give the final review

Apologies for missing this. I actually think @jhamman has done much more than I to contribute to this area!

But this change looks good, thanks @fujiisoup !

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Added a support for Dataset.rolling. 211323643
286665079 https://github.com/pydata/xarray/pull/1289#issuecomment-286665079 https://api.github.com/repos/pydata/xarray/issues/1289 MDEyOklzc3VlQ29tbWVudDI4NjY2NTA3OQ== jhamman 2443309 2017-03-15T07:45:51Z 2017-03-15T07:45:51Z MEMBER

@fujiisoup - This is looking pretty good. I'll let @MaximilianR give the final review since he's spent more time here than I have. As for your four points above, I think I'm fine with all of those design decisions.

One minor point though (no change needed)

All the attrs are dropped

I don't think this is true for data_vars that don't have rolling-dim in them.

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Added a support for Dataset.rolling. 211323643
284367479 https://github.com/pydata/xarray/pull/1289#issuecomment-284367479 https://api.github.com/repos/pydata/xarray/issues/1289 MDEyOklzc3VlQ29tbWVudDI4NDM2NzQ3OQ== fujiisoup 6815844 2017-03-06T11:08:09Z 2017-03-06T11:08:09Z MEMBER

we try to do it when possible because it leads to more predictable results.

OK. I totally agree with this. (Today I experienced a bug caused by unexpectedly aligned data_vars in other project. I realized predictable behavior is really important :) )

I think this PR is ready for review. If anyone has comments about the following behavior, I'm happy to update the code. + Only data_vars depending on the given rolling-dim are reduced. The others remain unchanged but are still stored in the reduced dataset (not dropped). + If rolling-dim is not included in Dataset.dims, this class raises KeyErrror. + The order of data_vars remain unchanged even after reduce(). + All the attrs are dropped.

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Added a support for Dataset.rolling. 211323643
284278244 https://github.com/pydata/xarray/pull/1289#issuecomment-284278244 https://api.github.com/repos/pydata/xarray/issues/1289 MDEyOklzc3VlQ29tbWVudDI4NDI3ODI0NA== shoyer 1217238 2017-03-06T00:49:22Z 2017-03-06T00:49:22Z MEMBER

I didn't notice Dataset keeps the order of data_vars.

It's not a strict requirement, but we try to do it when possible because it leads to more predictable results.

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Added a support for Dataset.rolling. 211323643
284226244 https://github.com/pydata/xarray/pull/1289#issuecomment-284226244 https://api.github.com/repos/pydata/xarray/issues/1289 MDEyOklzc3VlQ29tbWVudDI4NDIyNjI0NA== fujiisoup 6815844 2017-03-05T13:00:02Z 2017-03-05T13:02:00Z MEMBER
  • Now DatasetRolling.reduce() keeps its original data_vars order.
  • Dataset.rolling is added to api.rst.

I updated DatasetRolling so that its reduce method relies mostly on DataArrayRolling.reduce rather than window.reduce. This change makes not only the reduced-Dataset keep its original order of data_var, but also makes the code much cleaner. I moved many methods from Rolling to DataArrayRolling because DatasetRolling does not use them anymore.

However, I guess Rolling class is originally designed to be used also for Dataset, as discussed in #859. If someone has any requests, I will reconsider the class structure.

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Added a support for Dataset.rolling. 211323643
284217037 https://github.com/pydata/xarray/pull/1289#issuecomment-284217037 https://api.github.com/repos/pydata/xarray/issues/1289 MDEyOklzc3VlQ29tbWVudDI4NDIxNzAzNw== fujiisoup 6815844 2017-03-05T09:48:55Z 2017-03-05T09:48:55Z MEMBER

@shoyer Thanks for the review.

I didn't notice Dataset keeps the order of data_vars. I think I should change the direction, because the data_var's order is destroyed after the final merge call.

Maybe DatasetRolling should not be a child of Rolling, but an independent class that has an OrderedDict of Rolling for each data_var to preserve the variable order. 

{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Added a support for Dataset.rolling. 211323643
283931660 https://github.com/pydata/xarray/pull/1289#issuecomment-283931660 https://api.github.com/repos/pydata/xarray/issues/1289 MDEyOklzc3VlQ29tbWVudDI4MzkzMTY2MA== fujiisoup 6815844 2017-03-03T11:30:45Z 2017-03-03T11:30:45Z MEMBER

@MaximilianR

Thanks for the review. I made rolling_cls and groupby_cls private, and added some docstrings.

Also, I removed _axis_num property in Rolling class to make it compatible with Dataset. It can be replaced by da.get_axis_num(self.dim).

 
{
    "total_count": 1,
    "+1": 1,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Added a support for Dataset.rolling. 211323643

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]);