issue_comments

Table actions

6 rows where issue = 849315490 and user = 4160723 sorted by updated_at descending

✎ View and edit SQL

This data as json, CSV (advanced)

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

user 1

  • benbovy · 6 ✖

issue 1

  • Flexible indexes: add Index base class and xindexes properties · 6 ✖

author_association 1

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
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
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

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