issue_comments

Table actions

7 rows where issue = 1550792876 sorted by updated_at descending

✎ View and edit SQL

This data as json, CSV (advanced)

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

author_association 3

issue 1

  • Coordinates not deep copy · 7 ✖
id html_url issue_url node_id user created_at updated_at ▲ author_association body reactions performed_via_github_app issue
1428391853 https://github.com/pydata/xarray/issues/7463#issuecomment-1428391853 https://api.github.com/repos/pydata/xarray/issues/7463 IC_kwDOAMm_X85VI4et dcherian 2448579 2023-02-13T17:47:22Z 2023-02-13T17:47:22Z MEMBER

whether we should continue allowing IndexVariable data be updated in place via .data property. IMO we should really deprecate it

I agree. We don't allow it on .values anyway for this reason.

Now I see we already did this in https://github.com/pydata/xarray/pull/3862/files So I guess that got lost in the indexes refactor.

whether deep=True should deep copy the Xarray index objects

I would expect deep=True to deep-copy absolutely everything. That said if the Index objects are immutable then it doesn't matter?

On that pandas thread I see:

discovered that even if the id of the index was different on the copy, modifying the cp.index.to_numpy() values was corrupting the original.

Seems like we should all be setting the numpy data to be read-only with array.flags.WRITEABLE = False

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Coordinates not deep copy 1550792876
1427538729 https://github.com/pydata/xarray/issues/7463#issuecomment-1427538729 https://api.github.com/repos/pydata/xarray/issues/7463 IC_kwDOAMm_X85VFoMp benbovy 4160723 2023-02-13T08:31:49Z 2023-02-13T09:26:10Z MEMBER

There are two issues:

  • whether we should continue allowing IndexVariable data be updated in place via .data property. IMO we should really deprecate it, especially that now it is possible to have custom, possibly expensive index structures built from one or more coordinates.

  • whether deep=True should deep copy the Xarray index objects. I don't have strong opinion on this. There is a similar discussion on the pandas side: https://github.com/pandas-dev/pandas/issues/19862. I wonder if we reverted the change here because some high-level operations in Xarray were by default deep copying the indexes? I don't think we would want such behavior unless the user explicitly sets deep=True somewhere?

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Coordinates not deep copy 1550792876
1427503304 https://github.com/pydata/xarray/issues/7463#issuecomment-1427503304 https://api.github.com/repos/pydata/xarray/issues/7463 IC_kwDOAMm_X85VFfjI mgaspary 118520620 2023-02-13T08:02:55Z 2023-02-13T08:02:55Z NONE

Yes I think we should, but I might have missed the rationale behind allowing it if this is intentional.

EDIT: perhaps better to issue a warning first to avoid some breaking change. We could also try to fix it (make a deep copy) at the same time as deprecating it, but that might be tricky without again introducing performance regressions.

I would assume that deepcopy are completely going to copy the object, so if I change internal parts (like coordinates here), 1st object shall not change. It definitely affects the performance, but otherwise deepcopy is not deep anymore

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Coordinates not deep copy 1550792876
1426311006 https://github.com/pydata/xarray/issues/7463#issuecomment-1426311006 https://api.github.com/repos/pydata/xarray/issues/7463 IC_kwDOAMm_X85VA8de benbovy 4160723 2023-02-10T20:31:10Z 2023-02-10T20:38:48Z MEMBER

Yes I think we should, but I might have missed the rationale behind allowing it if this is intentional.

EDIT: perhaps better to issue a warning first to avoid some breaking change. We could also try to fix it (make a deep copy) at the same time as deprecating it, but that might be tricky without again introducing performance regressions.

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Coordinates not deep copy 1550792876
1426303358 https://github.com/pydata/xarray/issues/7463#issuecomment-1426303358 https://api.github.com/repos/pydata/xarray/issues/7463 IC_kwDOAMm_X85VA6l- headtr1ck 43316012 2023-02-10T20:27:10Z 2023-02-10T20:27:10Z COLLABORATOR

I didn't see #1463 (#1463 (comment)), though. It feels weird to me that we can mutate an IndexVariable via its data property, considering that the underlying index is immutable. IIUC xarr2.x.data[0] = 45 replaces the full index with a new one? I'm not sure if it is a good idea to allow this. For a pandas index that's probably OK (it is reasonably cheap to rebuild a new index) but for a custom index that is expensive to build (e.g., kd-tree) I don't think this behavior is desirable.

should we then raise an error if someone tries to replace values in an index?

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Coordinates not deep copy 1550792876
1426299770 https://github.com/pydata/xarray/issues/7463#issuecomment-1426299770 https://api.github.com/repos/pydata/xarray/issues/7463 IC_kwDOAMm_X85VA5t6 benbovy 4160723 2023-02-10T20:25:12Z 2023-02-10T20:25:12Z MEMBER

I think that the reverting change in IndexVariable came after refactoring copy in Xarray introduced some performance regression (https://github.com/pydata/xarray/pull/7209#issuecomment-1305593478).

I didn't see #1463 (https://github.com/pydata/xarray/issues/1463#issuecomment-340454702), though. It feels weird to me that we can mutate an IndexVariable via its data property, considering that the underlying index is immutable. IIUC xarr2.x.data[0] = 45 replaces the full index with a new one? I'm not sure if it is a good idea to allow this. For a pandas index that's probably OK (it is reasonably cheap to rebuild a new index) but for a custom index that is expensive to build (e.g., kd-tree) I don't think this behavior is desirable.

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Coordinates not deep copy 1550792876
1426267182 https://github.com/pydata/xarray/issues/7463#issuecomment-1426267182 https://api.github.com/repos/pydata/xarray/issues/7463 IC_kwDOAMm_X85VAxwu headtr1ck 43316012 2023-02-10T19:53:22Z 2023-02-10T19:53:22Z COLLABORATOR

It seems that in copy IndexVariables are treated special and are explicitly excluded from copying. @benbovy what was the reasoning behind this choice?

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Coordinates not deep copy 1550792876

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