issue_comments

Table actions

6 rows where issue = 587062505 sorted by updated_at descending

✎ View and edit SQL

This data as json, CSV (advanced)

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

user 3

author_association 2

issue 1

  • Allow for All-NaN in argmax, argmin · 6 ✖
id html_url issue_url node_id user created_at updated_at ▲ author_association body reactions performed_via_github_app issue
604838173 https://github.com/pydata/xarray/issues/3884#issuecomment-604838173 https://api.github.com/repos/pydata/xarray/issues/3884 MDEyOklzc3VlQ29tbWVudDYwNDgzODE3Mw== shoyer 1217238 2020-03-27T06:31:08Z 2020-03-27T06:31:08Z MEMBER

I wouldn’t worry too much about reusing bottleneck here, unless we really these functions will be the bottleneck in user code :)

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Allow for All-NaN in argmax, argmin 587062505
604775165 https://github.com/pydata/xarray/issues/3884#issuecomment-604775165 https://api.github.com/repos/pydata/xarray/issues/3884 MDEyOklzc3VlQ29tbWVudDYwNDc3NTE2NQ== toddrjen 2272878 2020-03-27T01:57:44Z 2020-03-27T01:57:44Z CONTRIBUTOR

@shoyer xarray uses bottleneck for that if it can in xarray.nputils, so copying the numpy method would result in a performance hit. However, xarray maintains a wrapper around the numpy/bottleneck version in xarray.nanops where this could perhaps be implemented.

 
{
    "total_count": 1,
    "+1": 1,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Allow for All-NaN in argmax, argmin 587062505
604225711 https://github.com/pydata/xarray/issues/3884#issuecomment-604225711 https://api.github.com/repos/pydata/xarray/issues/3884 MDEyOklzc3VlQ29tbWVudDYwNDIyNTcxMQ== shoyer 1217238 2020-03-26T04:42:37Z 2020-03-26T04:42:37Z MEMBER

NumPy implements nanargmax, including raising the error, in Python. It would be very doable to copy a modified version into xarray.

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Allow for All-NaN in argmax, argmin 587062505
604220730 https://github.com/pydata/xarray/issues/3884#issuecomment-604220730 https://api.github.com/repos/pydata/xarray/issues/3884 MDEyOklzc3VlQ29tbWVudDYwNDIyMDczMA== toddrjen 2272878 2020-03-26T04:22:18Z 2020-03-26T04:22:18Z CONTRIBUTOR

The problem I had when implementing idxmin and idxmax is that this behavior is defined by numpy, not by xarray, and bottleneck follows the same behavior, with xarray generally delegating the computation to one of these. So you would need to somehow work around the behavior of numpy in xarray or get a fix implemented both in numpy and bottleneck.

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Allow for All-NaN in argmax, argmin 587062505
604186105 https://github.com/pydata/xarray/issues/3884#issuecomment-604186105 https://api.github.com/repos/pydata/xarray/issues/3884 MDEyOklzc3VlQ29tbWVudDYwNDE4NjEwNQ== shoyer 1217238 2020-03-26T02:07:27Z 2020-03-26T02:07:27Z MEMBER

The main concern here is type stability. Normally the return value of argmax is an integer dtype array, but NaN isn't a valid integer :(

My suggestion would be to add an optional fill_value argument, similar to the API under discussion for idxmax in https://github.com/pydata/xarray/pull/3871. If fill_value is specified (e.g., with fill_value=np.nan or fill_value=-1) then missing values are returned with the fill value instead of raising an error.

 
{
    "total_count": 2,
    "+1": 2,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Allow for All-NaN in argmax, argmin 587062505
603341320 https://github.com/pydata/xarray/issues/3884#issuecomment-603341320 https://api.github.com/repos/pydata/xarray/issues/3884 MDEyOklzc3VlQ29tbWVudDYwMzM0MTMyMA== max-sixty 5635139 2020-03-24T16:18:54Z 2020-03-24T16:18:54Z MEMBER

I think this would be a reasonable change

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Allow for All-NaN in argmax, argmin 587062505

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