home / github

Menu
  • GraphQL API
  • Search all tables

issue_comments

Table actions
  • GraphQL API for issue_comments

14 rows where author_association = "MEMBER" and issue = 443157666 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 4

  • keewis 9
  • shoyer 3
  • dcherian 1
  • max-sixty 1

issue 1

  • Picking up #1118: Do not convert subclasses of `ndarray` unless required · 14 ✖

author_association 1

  • MEMBER · 14 ✖
id html_url issue_url node_id user created_at updated_at ▲ author_association body reactions performed_via_github_app issue
568903698 https://github.com/pydata/xarray/pull/2956#issuecomment-568903698 https://api.github.com/repos/pydata/xarray/issues/2956 MDEyOklzc3VlQ29tbWVudDU2ODkwMzY5OA== keewis 14808389 2019-12-25T14:11:45Z 2019-12-25T14:11:45Z MEMBER

Since we have been making a lot of progress using __array_function__ (which does not exclude subclasses but is much more flexible), this approach is outdated and I'm closing this and #1118.

{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
  Picking up #1118: Do not convert subclasses of `ndarray` unless required 443157666
524881974 https://github.com/pydata/xarray/pull/2956#issuecomment-524881974 https://api.github.com/repos/pydata/xarray/issues/2956 MDEyOklzc3VlQ29tbWVudDUyNDg4MTk3NA== keewis 14808389 2019-08-26T14:26:34Z 2019-08-26T14:26:34Z MEMBER

We could add the experimental option for allowing subclasses for testing purposes, but ultimately I think we want to insist that objects we use inside data implement __array_function__, regardless of whether they are a numpy array subclass or duck array. Otherwise there's no way complicated operations like concat are going to work properly.

@shoyer: What kind of testing would that be? If this is something that would be removed again in the future and has no actual value, feel free to close (but don't forget the original PR). Otherwise, I'd be happy to continue working on this.

{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
  Picking up #1118: Do not convert subclasses of `ndarray` unless required 443157666
523242520 https://github.com/pydata/xarray/pull/2956#issuecomment-523242520 https://api.github.com/repos/pydata/xarray/issues/2956 MDEyOklzc3VlQ29tbWVudDUyMzI0MjUyMA== keewis 14808389 2019-08-21T00:08:53Z 2019-08-21T00:08:53Z MEMBER

then I would like to finish this one and modify the tests in a new one.

{
    "total_count": 1,
    "+1": 1,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
  Picking up #1118: Do not convert subclasses of `ndarray` unless required 443157666
523232553 https://github.com/pydata/xarray/pull/2956#issuecomment-523232553 https://api.github.com/repos/pydata/xarray/issues/2956 MDEyOklzc3VlQ29tbWVudDUyMzIzMjU1Mw== shoyer 1217238 2019-08-20T23:21:42Z 2019-08-20T23:21:42Z MEMBER

You are welcome to continue here or in a new PR, whichever you prefer. I don't have a strong opinion, please do whichever seems easier for you.

I think the best examples of complex operations to check for compatibility can be found in xarray/tests/test_sparse.py and xarray/tests/test_dask.py

{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
  Picking up #1118: Do not convert subclasses of `ndarray` unless required 443157666
523231052 https://github.com/pydata/xarray/pull/2956#issuecomment-523231052 https://api.github.com/repos/pydata/xarray/issues/2956 MDEyOklzc3VlQ29tbWVudDUyMzIzMTA1Mg== keewis 14808389 2019-08-20T23:15:00Z 2019-08-20T23:15:00Z MEMBER

I think I would like to tackle this, but I definitely will need some help with finding examples for some of the more advanced operations.

@shoyer should I add these in a new PR or should I repurpose this one?

{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
  Picking up #1118: Do not convert subclasses of `ndarray` unless required 443157666
523229039 https://github.com/pydata/xarray/pull/2956#issuecomment-523229039 https://api.github.com/repos/pydata/xarray/issues/2956 MDEyOklzc3VlQ29tbWVudDUyMzIyOTAzOQ== keewis 14808389 2019-08-20T23:06:27Z 2019-08-20T23:07:04Z MEMBER

Do you have a rough idea of which operations these tests should check? The ones currently implemented are mean() and an xfailing concat. There is also the sel(), but as it only tries to use values with the matching unit, it works even though it should probably fail.

{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
  Picking up #1118: Do not convert subclasses of `ndarray` unless required 443157666
523225141 https://github.com/pydata/xarray/pull/2956#issuecomment-523225141 https://api.github.com/repos/pydata/xarray/issues/2956 MDEyOklzc3VlQ29tbWVudDUyMzIyNTE0MQ== shoyer 1217238 2019-08-20T22:50:25Z 2019-08-20T22:50:25Z MEMBER

However, the question of whether or not this PR is obsolete now still remains: if one were to replace quantities with pint, the test checking that this does not work without the proposed changes enabled is the only one that fails.

Does pint do __array_function__ yet? It looks like the answer is "not yet" but there's a branch with experimental support: https://github.com/hgrecco/pint/pull/764

We could add the experimental option for allowing subclasses for testing purposes, but ultimately I think we want to insist that objects we use inside data implement __array_function__, regardless of whether they are a numpy array subclass or duck array. Otherwise there's no way complicated operations like concat are going to work properly.

I'd love to add tests that verify that xarray can properly wrap arrays with units as soon as possible. We could even merge tests that are all marked "xfail" and that currently only pass when using the experimental version of pint from https://github.com/hgrecco/pint/pull/764.

As we saw with sparse array support, this will be useful for two reasons: 1. It will help us identify remaining issues that need to be solved either in xarray or in an array-with-units library like pint. 2. There are also likely some subtle tweaks we can do inside xarray to better support arrays with units, e.g., displaying units in the xarray.Dataset repr.

{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
  Picking up #1118: Do not convert subclasses of `ndarray` unless required 443157666
523183790 https://github.com/pydata/xarray/pull/2956#issuecomment-523183790 https://api.github.com/repos/pydata/xarray/issues/2956 MDEyOklzc3VlQ29tbWVudDUyMzE4Mzc5MA== keewis 14808389 2019-08-20T20:35:02Z 2019-08-20T20:35:02Z MEMBER

I'm not too sure about putting _asarray into npcompat.py, but apart from that I would say it is ready to be merged.

However, the question of whether or not this PR is obsolete now still remains: if one were to replace quantities with pint, the test checking that this does not work without the proposed changes enabled is the only one that fails.

{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
  Picking up #1118: Do not convert subclasses of `ndarray` unless required 443157666
520632305 https://github.com/pydata/xarray/pull/2956#issuecomment-520632305 https://api.github.com/repos/pydata/xarray/issues/2956 MDEyOklzc3VlQ29tbWVudDUyMDYzMjMwNQ== keewis 14808389 2019-08-12T23:32:50Z 2019-08-12T23:32:50Z MEMBER

now that #3117 has been merged, what is the status on this? From what I can tell from the few I looked at, most ndarray subclasses either already use NEP18 or will do so in the future. I guess what I want to know is: should I continue to work on it or does it overlap too much with that PR?

{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
  Picking up #1118: Do not convert subclasses of `ndarray` unless required 443157666
494163389 https://github.com/pydata/xarray/pull/2956#issuecomment-494163389 https://api.github.com/repos/pydata/xarray/issues/2956 MDEyOklzc3VlQ29tbWVudDQ5NDE2MzM4OQ== dcherian 2448579 2019-05-20T21:49:10Z 2019-05-20T21:49:10Z MEMBER

Looks like a glitch related to downloading an example file. Ignore it.

{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
  Picking up #1118: Do not convert subclasses of `ndarray` unless required 443157666
494151339 https://github.com/pydata/xarray/pull/2956#issuecomment-494151339 https://api.github.com/repos/pydata/xarray/issues/2956 MDEyOklzc3VlQ29tbWVudDQ5NDE1MTMzOQ== keewis 14808389 2019-05-20T21:07:06Z 2019-05-20T21:07:06Z MEMBER

I added the option, though the documentation is minimal. It gets used by a function to decide between asarray and asanyarray, and this function is then called in as_compatible_data and in the Variable.data property. Also, .mean() did not work before because duck_array_ops.asarray used np.asarray, which I replaced with that switching function. Should I test more of these reduce functions (if so, which)?

The matrix class gets treated similar to MaskedArray, and the unittests prove that enabling subclass support does not work for these two.

Is the failure of the docs related to any of the changes I made?

{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
  Picking up #1118: Do not convert subclasses of `ndarray` unless required 443157666
493839144 https://github.com/pydata/xarray/pull/2956#issuecomment-493839144 https://api.github.com/repos/pydata/xarray/issues/2956 MDEyOklzc3VlQ29tbWVudDQ5MzgzOTE0NA== shoyer 1217238 2019-05-20T04:59:16Z 2019-05-20T04:59:16Z MEMBER

This looks like a good start to me. So we can start making progress on this, I think it would make sense to hide this behavior behind an option, e.g., xarray.set_options(enable_experimental_ndarray_subclass_support=True).

I would consider writing some tests with two of the ndarray subclasses that ship with NumPy: - np.matrix - np.ma.MaskedArray

Both of these subclasses violate some important principles of subclassing, namely they aren't substitutable with objects of their parent class. Thus, I think it would be reasonable to explicitly "blacklist" them for use inside in xarray.DataArray. I think we already explicitly convert MaskedArray objects, but perhaps we should also add a check for np.matrix objects.

In terms of operations, one big class that comes to mind are aggregations, e.g., .mean(). It would be good to make sure that these work.

{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
  Picking up #1118: Do not convert subclasses of `ndarray` unless required 443157666
493803589 https://github.com/pydata/xarray/pull/2956#issuecomment-493803589 https://api.github.com/repos/pydata/xarray/issues/2956 MDEyOklzc3VlQ29tbWVudDQ5MzgwMzU4OQ== keewis 14808389 2019-05-20T00:01:26Z 2019-05-20T00:01:26Z MEMBER

Most of the items above are implemented in unit-support, even support for array wrappers with units like pint (if my tests are sufficient for deciding that: pint still complains/warns about stripped units). The calls to asarray and asanyarray are skipped based on whether any of the __array_*__ methods and the attributes unit or units exist.

I did not push these commits to this PR because most of it does not have anything to do with subclasses. I did not open a new PR for the branch either because it builds on this one and because I'd like to receive feedback on any of this first.

Combining this with dask arrays should still fail, though.

{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
  Picking up #1118: Do not convert subclasses of `ndarray` unless required 443157666
491641438 https://github.com/pydata/xarray/pull/2956#issuecomment-491641438 https://api.github.com/repos/pydata/xarray/issues/2956 MDEyOklzc3VlQ29tbWVudDQ5MTY0MTQzOA== max-sixty 5635139 2019-05-13T00:19:19Z 2019-05-13T00:19:19Z MEMBER

Looks great! Thanks @keewis

I don't know this area that well, so I'll defer to someone else on a proper review

{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
  Picking up #1118: Do not convert subclasses of `ndarray` unless required 443157666

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]);
Powered by Datasette · Queries took 13.388ms · About: xarray-datasette