issue_comments

Table actions

9 rows where issue = 308039063 sorted by updated_at descending

✖
✖

✎ View and edit SQL

This data as json, CSV (advanced)

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

author_association 2

issue 1

  • rolling: periodic · 9 ✖
id html_url issue_url node_id user created_at updated_at ▲ author_association body reactions performed_via_github_app issue
482403368 https://github.com/pydata/xarray/pull/2011#issuecomment-482403368 https://api.github.com/repos/pydata/xarray/issues/2011 MDEyOklzc3VlQ29tbWVudDQ4MjQwMzM2OA== shoyer 1217238 2019-04-12T01:43:07Z 2019-04-12T01:43:07Z MEMBER
  • is fill_value='periodic' a good api?

fill_value='periodic' almost suggests padding with the string 'periodic'. Maybe padding='periodic' vs padding=None (default)?

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   rolling: periodic 308039063
482402058 https://github.com/pydata/xarray/pull/2011#issuecomment-482402058 https://api.github.com/repos/pydata/xarray/issues/2011 MDEyOklzc3VlQ29tbWVudDQ4MjQwMjA1OA== shoyer 1217238 2019-04-12T01:36:11Z 2019-04-12T01:36:11Z MEMBER

It might make sense to start with the higher level pad() with periodic boundary conditions first (https://github.com/pydata/xarray/issues/2605). Then rolling with periodic boundary conditions is a simple matter of padding, then rolling.

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   rolling: periodic 308039063
482143007 https://github.com/pydata/xarray/pull/2011#issuecomment-482143007 https://api.github.com/repos/pydata/xarray/issues/2011 MDEyOklzc3VlQ29tbWVudDQ4MjE0MzAwNw== rabernat 1197350 2019-04-11T14:41:23Z 2019-04-11T14:41:23Z MEMBER

Rather than letting this PR lie dormant, maybe we should just move forward without dask support and raise an appropriate error if the user tries it with dask arrays.

 
{
    "total_count": 1,
    "+1": 1,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   rolling: periodic 308039063
392228821 https://github.com/pydata/xarray/pull/2011#issuecomment-392228821 https://api.github.com/repos/pydata/xarray/issues/2011 MDEyOklzc3VlQ29tbWVudDM5MjIyODgyMQ== raybellwaves 17162724 2018-05-26T02:22:07Z 2018-05-26T14:32:19Z CONTRIBUTOR

I may pick up this PR but will need some hand holding as i'm not familiar with dask.

I want to make sure I can do periodic rolling with dask in a simple script first but it needs work: https://gist.github.com/raybellwaves/621faaf195c0f4ed010b7b0dfee8605a

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   rolling: periodic 308039063
376145446 https://github.com/pydata/xarray/pull/2011#issuecomment-376145446 https://api.github.com/repos/pydata/xarray/issues/2011 MDEyOklzc3VlQ29tbWVudDM3NjE0NTQ0Ng== fujiisoup 6815844 2018-03-26T12:16:12Z 2018-03-26T12:16:12Z MEMBER

Ah, you are right. The only way to realize rolling.periodic with dask would be to manually attach a chunk in the other side to the boundaries, construct rolling window array and then remove unnecessary boundaries by slicing.

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   rolling: periodic 308039063
376122203 https://github.com/pydata/xarray/pull/2011#issuecomment-376122203 https://api.github.com/repos/pydata/xarray/issues/2011 MDEyOklzc3VlQ29tbWVudDM3NjEyMjIwMw== mathause 10194086 2018-03-26T10:37:00Z 2018-03-26T10:37:00Z MEMBER

Thanks for the discussion. I could not get it to work with dask - the problem is the 'padding before ghosting'.

https://github.com/pydata/xarray/blob/7c2c43ce6b1792e1193635ab9b64fd248266f632/xarray/core/dask_array_ops.py#L60-L62

I see no way how to get the boundary='periodic' working afterwards...

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   rolling: periodic 308039063
375835173 https://github.com/pydata/xarray/pull/2011#issuecomment-375835173 https://api.github.com/repos/pydata/xarray/issues/2011 MDEyOklzc3VlQ29tbWVudDM3NTgzNTE3Mw== fujiisoup 6815844 2018-03-24T01:07:32Z 2018-03-24T01:07:32Z MEMBER

Some comments.

  • We need to also support a dask array. Can you update dask_array_op.rolling_window? Maybe this page would help for the implementation.

We use this here https://github.com/pydata/xarray/blob/7c2c43ce6b1792e1193635ab9b64fd248266f632/xarray/core/dask_array_ops.py#L78-L79

I suspect this would be already working, but it would be nice if we can add some test cases.

  • Maybe we can also add boundary='reflect' option to make this as similar to dask.ghost.
 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   rolling: periodic 308039063
375833339 https://github.com/pydata/xarray/pull/2011#issuecomment-375833339 https://api.github.com/repos/pydata/xarray/issues/2011 MDEyOklzc3VlQ29tbWVudDM3NTgzMzMzOQ== fujiisoup 6815844 2018-03-24T00:45:25Z 2018-03-24T01:03:50Z MEMBER

Thanks for sending a PR.

is fill_value='periodic' a good api?

Maybe better API would be boundary='peiodic' (see comments), but changing the public API is not an easy task. Let's keep it now as is and decide it later.

should the fill_value keyvalue be ported to rolling?

I don't think so. I think we can keep rolling as similar to that of pandas, and this option should be specific to construct method.

should this also be mentioned in the docs for rolling (I only learned about rolling.construct yesterday)

I think at least we need to add this to the docstring of construct.

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   rolling: periodic 308039063
375677083 https://github.com/pydata/xarray/pull/2011#issuecomment-375677083 https://api.github.com/repos/pydata/xarray/issues/2011 MDEyOklzc3VlQ29tbWVudDM3NTY3NzA4Mw== mathause 10194086 2018-03-23T14:10:08Z 2018-03-23T14:50:02Z MEMBER

We would probably have to test how min_periods and periodic play together..., see #2010

edit: or not, as min_periods is not used in construct

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   rolling: periodic 308039063

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