issue_comments

Table actions

12 rows where issue = 582708100 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

  • Raise error when assigning to IndexVariable.values & IndexVariable.data · 12 ✖
id html_url issue_url node_id user created_at updated_at ▲ author_association body reactions performed_via_github_app issue
612145002 https://github.com/pydata/xarray/pull/3862#issuecomment-612145002 https://api.github.com/repos/pydata/xarray/issues/3862 MDEyOklzc3VlQ29tbWVudDYxMjE0NTAwMg== shoyer 1217238 2020-04-10T17:56:04Z 2020-04-10T17:59:44Z MEMBER

In light of #3470 (comment) it seems like the next best thing would be to given assign_coords the optional ability to update the DataArray/DataSet in-place. Thoughts?

There's no need to use assign_coords. You can also just use normal assignment to a DataArray/Dataset or coords, e.g, python ds['time'] = new_values # good ds.coords['time'] = new_values # good ds.coords['time'].values = new_values # broken, disabled by this PR ds.coords['time'].values[:] = new_values # also buggy (but harder to disable)

This would probably be worth clarifying in the error message.

(Under the hood, assign_coords basically does the exactly same thing as assignment to coords, it just copies the Dataset/DataArray` first)

 
{
    "total_count": 2,
    "+1": 2,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Raise error when assigning to IndexVariable.values & IndexVariable.data 582708100
612146195 https://github.com/pydata/xarray/pull/3862#issuecomment-612146195 https://api.github.com/repos/pydata/xarray/issues/3862 MDEyOklzc3VlQ29tbWVudDYxMjE0NjE5NQ== rsokl 29104956 2020-04-10T17:58:50Z 2020-04-10T17:58:50Z NONE

Ah! Nice! Yeah the error message led me to believe that there was no path for in-place updates.

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Raise error when assigning to IndexVariable.values & IndexVariable.data 582708100
612141362 https://github.com/pydata/xarray/pull/3862#issuecomment-612141362 https://api.github.com/repos/pydata/xarray/issues/3862 MDEyOklzc3VlQ29tbWVudDYxMjE0MTM2Mg== rsokl 29104956 2020-04-10T17:47:14Z 2020-04-10T17:47:14Z NONE

That's exactly what this change does, making assignment to IndexVariable.values an error.

Sorry, I got a bit criss-crossed between this PR and the thread in #3470.

In light of https://github.com/pydata/xarray/issues/3470#issuecomment-566214653 it seems like the next best thing would be to given assign_coords the optional ability to update the DataArray/DataSet in-place. Thoughts? 

{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Raise error when assigning to IndexVariable.values & IndexVariable.data 582708100
612132737 https://github.com/pydata/xarray/pull/3862#issuecomment-612132737 https://api.github.com/repos/pydata/xarray/issues/3862 MDEyOklzc3VlQ29tbWVudDYxMjEzMjczNw== shoyer 1217238 2020-04-10T17:26:40Z 2020-04-10T17:26:40Z MEMBER

is there a reason why this change wasn't incorporated into values.setter?

I'm not quite sure what you mean here. That's exactly what this change does, making assignment to IndexVariable.values an error.

If you were mutating .values on an indexed variable, then you already had a likely bug, because only part of xarray's data structures were being updated (the Variable, but not indexes). That's why we put this in as a bug fix -- see the linked issue https://github.com/pydata/xarray/issues/3470 for examples.

Unfortunately we couldn't figure out another way to make this work -- see https://github.com/pydata/xarray/issues/3470#issuecomment-566214653 for discussion. We're definitely open to alternatives if you can come up with them, though!

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Raise error when assigning to IndexVariable.values & IndexVariable.data 582708100
612095122 https://github.com/pydata/xarray/pull/3862#issuecomment-612095122 https://api.github.com/repos/pydata/xarray/issues/3862 MDEyOklzc3VlQ29tbWVudDYxMjA5NTEyMg== rsokl 29104956 2020-04-10T15:59:41Z 2020-04-10T15:59:41Z NONE

@dcherian @max-sixty is there a reason why this change wasn't incorporated into values.setter? This change blocks the ability to update coordinate values in-place. This breaks code in a more fundamental way than just requiring users to rely on assign_coords - it can necessitate a substantial change in code design for people who were relying on the ability to mutate coordinates. This also leads me to ask: was it really appropriate to call such a substantial breaking change only a patch? 

{
    "total_count": 1,
    "+1": 1,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Raise error when assigning to IndexVariable.values & IndexVariable.data 582708100
602596404 https://github.com/pydata/xarray/pull/3862#issuecomment-602596404 https://api.github.com/repos/pydata/xarray/issues/3862 MDEyOklzc3VlQ29tbWVudDYwMjU5NjQwNA== dcherian 2448579 2020-03-23T13:42:44Z 2020-03-23T13:42:44Z MEMBER

Great. Thanks all.

Now to find out how many downstream projects test against master on CI :)

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Raise error when assigning to IndexVariable.values & IndexVariable.data 582708100
602299713 https://github.com/pydata/xarray/pull/3862#issuecomment-602299713 https://api.github.com/repos/pydata/xarray/issues/3862 MDEyOklzc3VlQ29tbWVudDYwMjI5OTcxMw== spencerkclark 6628425 2020-03-22T23:47:45Z 2020-03-22T23:47:45Z MEMBER

I think now that you merged #3874, if you re-merge master they should pass.

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Raise error when assigning to IndexVariable.values & IndexVariable.data 582708100
602291813 https://github.com/pydata/xarray/pull/3862#issuecomment-602291813 https://api.github.com/repos/pydata/xarray/issues/3862 MDEyOklzc3VlQ29tbWVudDYwMjI5MTgxMw== dcherian 2448579 2020-03-22T23:14:03Z 2020-03-22T23:14:03Z MEMBER

I think the failed tests are unrelated since they only fail on the upstream-dev

FAILED xarray/tests/test_cftimeindex.py::test_indexing_in_series_getitem[365_day] FAILED xarray/tests/test_cftimeindex.py::test_indexing_in_series_getitem[360_day] FAILED xarray/tests/test_cftimeindex.py::test_indexing_in_series_getitem[julian] FAILED xarray/tests/test_cftimeindex.py::test_indexing_in_series_getitem[all_leap] FAILED xarray/tests/test_cftimeindex.py::test_indexing_in_series_getitem[366_day] FAILED xarray/tests/test_cftimeindex.py::test_indexing_in_series_getitem[gregorian] FAILED xarray/tests/test_cftimeindex.py::test_indexing_in_series_getitem[proleptic_gregorian]

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Raise error when assigning to IndexVariable.values & IndexVariable.data 582708100
602250092 https://github.com/pydata/xarray/pull/3862#issuecomment-602250092 https://api.github.com/repos/pydata/xarray/issues/3862 MDEyOklzc3VlQ29tbWVudDYwMjI1MDA5Mg== keewis 14808389 2020-03-22T18:17:09Z 2020-03-22T18:17:09Z MEMBER 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Raise error when assigning to IndexVariable.values & IndexVariable.data 582708100
602088059 https://github.com/pydata/xarray/pull/3862#issuecomment-602088059 https://api.github.com/repos/pydata/xarray/issues/3862 MDEyOklzc3VlQ29tbWVudDYwMjA4ODA1OQ== dcherian 2448579 2020-03-21T19:02:04Z 2020-03-21T19:02:04Z MEMBER

I think @shoyer should confirm that this is what we want to do

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Raise error when assigning to IndexVariable.values & IndexVariable.data 582708100
602080002 https://github.com/pydata/xarray/pull/3862#issuecomment-602080002 https://api.github.com/repos/pydata/xarray/issues/3862 MDEyOklzc3VlQ29tbWVudDYwMjA4MDAwMg== max-sixty 5635139 2020-03-21T17:59:13Z 2020-03-21T17:59:13Z MEMBER

OK to merge @dcherian ? We can almost release 0.15.1 then

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Raise error when assigning to IndexVariable.values & IndexVariable.data 582708100
600127485 https://github.com/pydata/xarray/pull/3862#issuecomment-600127485 https://api.github.com/repos/pydata/xarray/issues/3862 MDEyOklzc3VlQ29tbWVudDYwMDEyNzQ4NQ== max-sixty 5635139 2020-03-17T15:16:32Z 2020-03-17T15:16:32Z MEMBER

Great, good spot @dcherian 

{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Raise error when assigning to IndexVariable.values & IndexVariable.data 582708100

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