issue_comments

Table actions

10 rows where issue = 849315490 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

  • Flexible indexes: add Index base class and xindexes properties · 10 ✖
id html_url issue_url node_id user created_at updated_at ▲ author_association body reactions performed_via_github_app issue
838055686 https://github.com/pydata/xarray/pull/5102#issuecomment-838055686 https://api.github.com/repos/pydata/xarray/issues/5102 MDEyOklzc3VlQ29tbWVudDgzODA1NTY4Ng== benbovy 4160723 2021-05-11T08:21:03Z 2021-05-11T08:21:03Z MEMBER

All right let's merge this! Thanks everyone for your review comments.

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Flexible indexes: add Index base class and xindexes properties 849315490
829272836 https://github.com/pydata/xarray/pull/5102#issuecomment-829272836 https://api.github.com/repos/pydata/xarray/issues/5102 MDEyOklzc3VlQ29tbWVudDgyOTI3MjgzNg== pep8speaks 24736507 2021-04-29T14:13:05Z 2021-05-10T15:16:46Z NONE

Hello @benbovy! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers:

Comment last updated at 2021-05-10 15:16:46 UTC
 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Flexible indexes: add Index base class and xindexes properties 849315490
836317739 https://github.com/pydata/xarray/pull/5102#issuecomment-836317739 https://api.github.com/repos/pydata/xarray/issues/5102 MDEyOklzc3VlQ29tbWVudDgzNjMxNzczOQ== shoyer 1217238 2021-05-10T07:49:58Z 2021-05-10T07:49:58Z MEMBER

(Feel free to self-merge after fixing the merge conflict! My suggested fix can be done later, I don't want this to block you)

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Flexible indexes: add Index base class and xindexes properties 849315490
836278394 https://github.com/pydata/xarray/pull/5102#issuecomment-836278394 https://api.github.com/repos/pydata/xarray/issues/5102 MDEyOklzc3VlQ29tbWVudDgzNjI3ODM5NA== benbovy 4160723 2021-05-10T07:12:36Z 2021-05-10T07:12:36Z MEMBER

Should we merge this?

In follow-up PRs, I plan to:

  • Create a PandasMultiIndex class and refactor all places in Xarray that rely on pd.MultiIndex (internal changes only).
  • Add public API to Xarray Index for label-based data selection and refactor/move the logic in convert_label_indexer into the Xarray PandasIndex and PandasMultiIndex classes.
 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Flexible indexes: add Index base class and xindexes properties 849315490
831789860 https://github.com/pydata/xarray/pull/5102#issuecomment-831789860 https://api.github.com/repos/pydata/xarray/issues/5102 MDEyOklzc3VlQ29tbWVudDgzMTc4OTg2MA== benbovy 4160723 2021-05-04T09:01:10Z 2021-05-04T09:01:10Z MEMBER

Thanks for the review @shoyer, I addressed your comments.

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Flexible indexes: add Index base class and xindexes properties 849315490
830150205 https://github.com/pydata/xarray/pull/5102#issuecomment-830150205 https://api.github.com/repos/pydata/xarray/issues/5102 MDEyOklzc3VlQ29tbWVudDgzMDE1MDIwNQ== benbovy 4160723 2021-04-30T14:53:12Z 2021-04-30T14:53:12Z MEMBER

This is ready for review!

I implemented @shoyer's suggestions and finished updating the (many) places where Xarray directly uses pandas indexes.

I also added a to_pandas_index() method to the Index base class so that we can easily raise when an Xarray operation doesn't support flexible indexes (i.e., indexes that cannot be cast to a pd.Index).

There might still be some quick fixes that we could clean up now, but I think that most things will have to be cleaned up later in the refactoring once additional features/classes/etc. are implemented.

I haven't wrote tests for the Index base class yet as this is still very much preliminary work, I plan to do it in follow-up PRs once the API becomes more stable. 

{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Flexible indexes: add Index base class and xindexes properties 849315490
814034661 https://github.com/pydata/xarray/pull/5102#issuecomment-814034661 https://api.github.com/repos/pydata/xarray/issues/5102 MDEyOklzc3VlQ29tbWVudDgxNDAzNDY2MQ== benbovy 4160723 2021-04-06T11:09:38Z 2021-04-06T11:09:38Z MEMBER

Agreed for adding another property along with a couple of depreciation cycles for smooth transition on what is returned by .indexes. I've suggested .index_adapters in my previous comment but I prefer .xindexes.

xarray.Index makes sense too. For subclasses wrapping another index class perhaps it might be still be relevant to use the Adapter suffix, for example to distinguish between a pandas.Index object and a xarray.PandasIndexAdapter object.

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Flexible indexes: add Index base class and xindexes properties 849315490
813090844 https://github.com/pydata/xarray/pull/5102#issuecomment-813090844 https://api.github.com/repos/pydata/xarray/issues/5102 MDEyOklzc3VlQ29tbWVudDgxMzA5MDg0NA== shoyer 1217238 2021-04-04T19:56:51Z 2021-04-04T19:56:51Z MEMBER

Rather than xarray.IndexAdapter, maybe we should just call this new object xarray.Index? Calling this object an "adapter" diminishing its importance in Xarray's future API.

I agree that switching the return type of .indexes is probably worthy of a breaking change -- but that breaking change should be done intentionally, once the new indexing functionality works and we are ready to make a major release. We may also want a deprecation cycle. What we don't want to do is change things in an incomplete way now, in a way that makes it hard for us to issue a bug-fix release.

To make development easier, I would suggest adding a new attribute to xarray.Dataset and xarray.DataArray that exposes the new data model, e.g., perhaps .xindexes as short for "xarray indexes". We would then: 1. Immediately switch xarray to use .xindexes instead of .indexes internally. 2. Once the new indexing functionality is ready, encourage users to gradually switch from .indexes -> .xindexes by issuing a FutureWarning warning. 3. After an appropriate period of time, consider making .indexes an alias for .xindexes in a breaking release.

 
{
    "total_count": 2,
    "+1": 2,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Flexible indexes: add Index base class and xindexes properties 849315490
812836043 https://github.com/pydata/xarray/pull/5102#issuecomment-812836043 https://api.github.com/repos/pydata/xarray/issues/5102 MDEyOklzc3VlQ29tbWVudDgxMjgzNjA0Mw== benbovy 4160723 2021-04-03T08:46:23Z 2021-04-03T08:46:23Z MEMBER

maybe .indexes should continue to return pandas.Index objects for now, by unwrapped IndexAdapters sorted in ._indexes?

Yes we could make a special case for pandas indexes. This would also make the refactoring easier now since .indexes is used internally in quite many places that expect pandas.Index objects. Not sure if it's a good solution in the mid/long term, though.

For example, it would be nice to move the logic implemented in convert_label_indexer into PandasIndexAdapter and PandasMultiIndexAdapter classes and call methods of those classes instead of dealing directly with pandas index objects. For this example (and perhaps others) we could use _indexes internally. Alternatively it might make sense to have an .index_adapters property to make things a bit clearer (this may be welcome even for internal purpose IMO).

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Flexible indexes: add Index base class and xindexes properties 849315490
812753940 https://github.com/pydata/xarray/pull/5102#issuecomment-812753940 https://api.github.com/repos/pydata/xarray/issues/5102 MDEyOklzc3VlQ29tbWVudDgxMjc1Mzk0MA== shoyer 1217238 2021-04-02T23:30:53Z 2021-04-02T23:30:53Z MEMBER
  • the xarray_obj.indexes properties now returns IndexAdapter (PandasIndexAdapter) instances instead of pandas.Index instances

The latter is a breaking change, although I'm not sure if the indexes property has been made public yet.

This is indeed unfortunately a public API, so we should think about how to roll this out with minimal disruption.

For example: maybe .indexes should continue to return pandas.Index objects for now, by unwrapped IndexAdapters sorted in ._indexes?

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Flexible indexes: add Index base class and xindexes properties 849315490

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