home / github

Menu
  • Search all tables
  • GraphQL API

issue_comments

Table actions
  • GraphQL API for issue_comments

where issue = 169588316 and user = 1217238 sorted by updated_at descending

✖
✖
✖

✎ View and edit SQL

This data as json, CSV (advanced)

These facets timed out: author_association, issue

user 1

  • shoyer · 9 ✖
id html_url issue_url node_id user created_at updated_at ▲ author_association body reactions performed_via_github_app issue
246896876 https://github.com/pydata/xarray/pull/947#issuecomment-246896876 https://api.github.com/repos/pydata/xarray/issues/947 MDEyOklzc3VlQ29tbWVudDI0Njg5Njg3Ng== shoyer 1217238 2016-09-14T03:35:04Z 2016-09-14T03:35:04Z MEMBER

OK, in it goes. Big thanks to @benbovy !

{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
  Multi-index levels as coordinates 169588316
246560708 https://github.com/pydata/xarray/pull/947#issuecomment-246560708 https://api.github.com/repos/pydata/xarray/issues/947 MDEyOklzc3VlQ29tbWVudDI0NjU2MDcwOA== shoyer 1217238 2016-09-13T03:08:37Z 2016-09-13T03:08:37Z MEMBER

I think the main (only?) thing left to do here is to remove the name argument you added to IndexVariable. For now, we can live with some IndexVariable objects with an inconsistent name (I'll remove the attribute shortly).

{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
  Multi-index levels as coordinates 169588316
244518196 https://github.com/pydata/xarray/pull/947#issuecomment-244518196 https://api.github.com/repos/pydata/xarray/issues/947 MDEyOklzc3VlQ29tbWVudDI0NDUxODE5Ng== shoyer 1217238 2016-09-03T01:14:48Z 2016-09-03T01:14:48Z MEMBER

If we remove IndexVariable.name, then we can modify IndexVariable.get_level_variable such that it accepts one or more level names and returns an OrderedDict of {level_name: IndexVariable} items. That would be even better than the current implementation. Maybe we should then rename get_level_variable to get_levels_as_variables?

I think what get_level_variable returns is an independent matter? I don't think it would be good idea to return a dict here because computing level values can be expensive and the levels aren't always necessary (they're stored internally as an Index for each level and integer codes).

BTW, I will be away over the holiday weekend (in the US), but I expect we will probably be able to merge this shortly after I get back.

{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
  Multi-index levels as coordinates 169588316
244512583 https://github.com/pydata/xarray/pull/947#issuecomment-244512583 https://api.github.com/repos/pydata/xarray/issues/947 MDEyOklzc3VlQ29tbWVudDI0NDUxMjU4Mw== shoyer 1217238 2016-09-02T23:57:26Z 2016-09-02T23:57:26Z MEMBER

Rather than adding an independent name attribute to IndexVariable, let's deprecate/remove IndexVariable.name instead (I can do this in a follow-up PR). Clearly it's useful to be able to use IndexVariable objects even for objects that do not correspond to ticks along a dimension, but for such objects, name is misleading. IndexVariable.name was never more than a convenient shortcut, and it's outlived it's usefulness.

{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
  Multi-index levels as coordinates 169588316
244142205 https://github.com/pydata/xarray/pull/947#issuecomment-244142205 https://api.github.com/repos/pydata/xarray/issues/947 MDEyOklzc3VlQ29tbWVudDI0NDE0MjIwNQ== shoyer 1217238 2016-09-01T16:55:12Z 2016-09-01T16:55:12Z MEMBER

Should I write new tests specific to multi-index or should I modify existing tests (e.g., TestDataset.test_modify_inplace, TestDataset.tes_coord_properties, TestDataset.test_coords_modify, etc.) to include the multi-index case?

I would usually lean towards dedicated tests (e.g., test_modify_multiindex_inplace) but if it's easier to modify the original tests in a small way (e.g., for repr) feel free to take that approach

{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
  Multi-index levels as coordinates 169588316
238709101 https://github.com/pydata/xarray/pull/947#issuecomment-238709101 https://api.github.com/repos/pydata/xarray/issues/947 MDEyOklzc3VlQ29tbWVudDIzODcwOTEwMQ== shoyer 1217238 2016-08-09T22:14:49Z 2016-08-09T22:14:49Z MEMBER

As a recent xarray user, I indeed remember that I initially found confusing to have Dataset or DataArray "coordinates" that can be either Coordinate or Variable objects. I find that the name IndexVariable is more representative of the object.

Sounds good, I will do this in a separate PR.

{
    "total_count": 1,
    "+1": 1,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
  Multi-index levels as coordinates 169588316
238053642 https://github.com/pydata/xarray/pull/947#issuecomment-238053642 https://api.github.com/repos/pydata/xarray/issues/947 MDEyOklzc3VlQ29tbWVudDIzODA1MzY0Mg== shoyer 1217238 2016-08-06T23:07:25Z 2016-08-06T23:07:25Z MEMBER

I'm conflicted about how to handle the repr. On the one hand, I like how * indicates indexable variables. On the other hand, it should indeed be clear that these are MultiIndex levels, not dimensions in their own right (especially if they don't appear in ds.coords.keys() and the like). So maybe something closer to what we had before would be better.

Let me try to sketch out some concrete proposals to encourage the peanut gallery to speak up:

Option 1: no special indicator for the MultiIndex:

Coordinates: * level_1 (x) object 'a' 'a' 'b' 'b' * level_2 (x) int64 0 1 0 1 * y (y) int64 0 1 2 3

Option 2: both MultiIndex and levels in repr:

Coordinates: * x (x) MultiIndex * level_1 (x) object 'a' 'a' 'b' 'b' * level_2 (x) int64 0 1 0 1 * y (y) int64 0 1 2 3

Option 3: both MultiIndex and levels in repr, different symbol for levels:

Coordinates: * x (x) MultiIndex - level_1 (x) object 'a' 'a' 'b' 'b' - level_2 (x) int64 0 1 0 1 * y (y) int64 0 1 2 3

Option 4: both MultiIndex and levels in repr, different symbol for levels, with indentation:

Coordinates: * x (x) MultiIndex - level_1 (x) object 'a' 'a' 'b' 'b' - level_2 (x) int64 0 1 0 1 * y (y) int64 0 1 2 3

A separate question (if we pick one of options 2-4) is how to represent the MultiIndex dtype and values (everything after the dimension name):

Option A: MultiIndex (as shown above) Option B: MultiIndex[level_0, level_1] Option C: object MultiIndex Option D: object MultiIndex[level_0, level_1] Option E: MultiIndex ('a', 0) ('a', 1) ('b', 0) ('b', 1) Option F: object ('a', 0) ('a', 1) ('b', 0) ('b', 1) (current repr)

The tradeoffs here are whether or not we include the exact dtype information (object), and how explicitly/redundantly we display the values.

I'm currently leaning toward Option 3A, but I don't have a strong opinion.

{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
  Multi-index levels as coordinates 169588316
238053437 https://github.com/pydata/xarray/pull/947#issuecomment-238053437 https://api.github.com/repos/pydata/xarray/issues/947 MDEyOklzc3VlQ29tbWVudDIzODA1MzQzNw== shoyer 1217238 2016-08-06T23:00:17Z 2016-08-06T23:02:10Z MEMBER

I would suggest putting the logic to create new variables for levels in the private _get_virtual_variable in dataset.py. We already call this function for creating variables on demand in operations like ds['time.month'], so it's already called in all the right places (even in ds.coords, and so on). We could simply extend it to also check for MultiIndex levels and build those variables on demand, too, but only when necessary. If possible, it would be nice if ds['time.day'] works even if time is a multi-index level.

This could get us most of the way there, but there are still a few things to watch out for: 1. What happens when you write ds.coords['level_1'] = ...? With the current implementation, I think this would create a new variable level_1. In an ideal world, maybe this would replace the MultiIndex level? For now, it is probably better to raise an error and note that the MultiIndex should be modified instead. 2. Should levels appear in ds.keys() or ds.coords.keys()? If we're printing them in the repr as peers to dimension coordinates, then maybe the answer should be yes? It could be confusing to have both the redundant levels and the multi-index coordinate in there, though. So maybe it's simpler to avoid changes here.

{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
  Multi-index levels as coordinates 169588316
238050009 https://github.com/pydata/xarray/pull/947#issuecomment-238050009 https://api.github.com/repos/pydata/xarray/issues/947 MDEyOklzc3VlQ29tbWVudDIzODA1MDAwOQ== shoyer 1217238 2016-08-06T21:35:24Z 2016-08-06T21:35:24Z MEMBER

This is very exciting to see!

A few thoughts on implementation:

Instead of always creating a dictionary of level coordinates, I would add an attribute level_names to Coordinate, which would default to None and be set to a tuple of MultiIndex.names if a Coordinate is created from a MultiIndex. This would make make checking for multi-index levels very cheap, even if we do need to iterate through every coordinate to find them.

It's much cheaper to call .get_level_values() after slicing a MultiIndex than before, e.g.,

``` In [12]: idx = pd.MultiIndex.from_product([np.linspace(0, 1, num=500), np.arange(1000)])

In [13]: %timeit idx.get_level_values(0)[:10] 1000 loops, best of 3: 1.28 ms per loop

In [14]: %timeit idx[:10].get_level_values(0) 10000 loops, best of 3: 101 µs per loop ```

It's even more extreme for larger indexes. If possible, we should use something closer to this approach when formatting coordinates.

However, it is not possible anymore to mix level indexers and non-dict dimension indexers, e.g., da.sel(x='a', level_2=1) doesn't work but da.sel(x={'level_1': 'a'}, level_2=1) does, which is already quite flexible though.

I would actually be happy to disallow both, which might be even easy. It seems like a fine rule to say that you cannot call .sel on both a level and the dimension name at the same time. Actually, if we check uniqueness of level names at Dataset/DataArray creation (which is a good idea!), there is not much need for level indexing with a dictionary at all.

{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
  Multi-index levels as coordinates 169588316

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 7117.898ms · About: xarray-datasette
  • Sort ascending
  • Sort descending
  • Facet by this
  • Hide this column
  • Show all columns
  • Show not-blank rows