issue_comments

Table actions

6 rows where issue = 497416198 sorted by updated_at descending

✎ View and edit SQL

This data as json, CSV (advanced)

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

user 2

author_association 2

issue 1

  • Provide better error message when dimension name matches argument · 6 ✖
id html_url issue_url node_id user created_at updated_at ▲ author_association body reactions performed_via_github_app issue
569074436 https://github.com/pydata/xarray/pull/3336#issuecomment-569074436 https://api.github.com/repos/pydata/xarray/issues/3336 MDEyOklzc3VlQ29tbWVudDU2OTA3NDQzNg== gwgundersen 2818208 2019-12-26T15:14:45Z 2019-12-26T15:14:45Z CONTRIBUTOR

The more I think about this PR, the more I dislike this approach. The solution must either be brittle or over-engineered. I discussed this issue with a friend, and other approaches don't seem better: currying the function—arr.sel(method="nearest")(method="foo")—or adding a decorator that caches the results of a stripped down inspect.signature. I think the best approach is to just raise a ValueError if certain string arguments are not in a predefined set of choices:

if method not in ["nearest", ...]: raise ValueError(...)

@max-sixty, @dcherian, @shoyer thoughts?

Not to open a can of worms, but the root cause of this issue is that the Xarray API accepts both **kwargs and ordinary named args. This PR—and methods like either_dict_or_kwargs—seems like workarounds to accommodate this model. Is there any interest in moving away from this API long-term?

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Provide better error message when dimension name matches argument 497416198
566861922 https://github.com/pydata/xarray/pull/3336#issuecomment-566861922 https://api.github.com/repos/pydata/xarray/issues/3336 MDEyOklzc3VlQ29tbWVudDU2Njg2MTkyMg== max-sixty 5635139 2019-12-18T04:27:11Z 2019-12-18T04:27:11Z MEMBER

@gwgundersen I'm really sorry to have kept you waiting, especially after this PR and the others strong ones you've recently done. I temporarily paused my xarray oversight while I came up to speed with a new job and let this drop.

@dcherian any chance you could take a glance? Otherwise I'll look properly this weekend. Thanks.

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Provide better error message when dimension name matches argument 497416198
564076665 https://github.com/pydata/xarray/pull/3336#issuecomment-564076665 https://api.github.com/repos/pydata/xarray/issues/3336 MDEyOklzc3VlQ29tbWVudDU2NDA3NjY2NQ== gwgundersen 2818208 2019-12-10T15:08:55Z 2019-12-10T15:08:55Z CONTRIBUTOR

I'm almost done with classes and am happy to return to this if there is any interest. I don't want to design something brittle, though, and would love some guidance.

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Provide better error message when dimension name matches argument 497416198
536231394 https://github.com/pydata/xarray/pull/3336#issuecomment-536231394 https://api.github.com/repos/pydata/xarray/issues/3336 MDEyOklzc3VlQ29tbWVudDUzNjIzMTM5NA== max-sixty 5635139 2019-09-28T22:55:00Z 2019-09-28T22:55:00Z MEMBER

Another alternative is a decorator that can encapsulate some of this meta-programming (could also do the work of either_dict_or_kwargs)

But if we're only doing this for a couple of methods, I would advocate we just write out the keywords. I know it's a bit of a inelegant defeat, but it's pythonic.

sel & isel are probably more important than the others

@shoyer do you have thoughts re encapsulation vs magic?

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Provide better error message when dimension name matches argument 497416198
536231119 https://github.com/pydata/xarray/pull/3336#issuecomment-536231119 https://api.github.com/repos/pydata/xarray/issues/3336 MDEyOklzc3VlQ29tbWVudDUzNjIzMTExOQ== max-sixty 5635139 2019-09-28T22:48:35Z 2019-09-28T22:48:35Z MEMBER

I did a basic performance test, and locals() is 1/100th of a second faster on average.

Do you know what the absolute numbers are? i.e. how much performance penalty does the faster routine introduce?

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Provide better error message when dimension name matches argument 497416198
534730840 https://github.com/pydata/xarray/pull/3336#issuecomment-534730840 https://api.github.com/repos/pydata/xarray/issues/3336 MDEyOklzc3VlQ29tbWVudDUzNDczMDg0MA== max-sixty 5635139 2019-09-24T20:20:08Z 2019-09-24T20:20:08Z MEMBER

That's a clever approach. It's a bit magic, but maybe the right tradeoff. Are there any performance implications?

I agree with @gwgundersen re writing each item out being impractical, unless we're only doing for sel & isel.

Any broader thoughts re the need to apply to all methods, vs this sort of magic?

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Provide better error message when dimension name matches argument 497416198

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