issue_comments

Table actions

5 rows where issue = 1359368857 sorted by updated_at descending

✎ View and edit SQL

This data as json, CSV (advanced)

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

author_association 3

issue 1

  • fix passing of curvefit kwargs · 5 ✖
id html_url issue_url node_id user created_at updated_at ▲ author_association body reactions performed_via_github_app issue
1261259923 https://github.com/pydata/xarray/pull/6978#issuecomment-1261259923 https://api.github.com/repos/pydata/xarray/issues/6978 IC_kwDOAMm_X85LLUyT dcherian 2448579 2022-09-28T17:50:56Z 2022-09-28T17:50:56Z MEMBER

only a dict of kwargs can be provided

I think we may want to keep only the current behaviour (perhaps add an example to the docstring instead), and have that be consistent across the project when we wrap external functions. It's similar to what we do with apply_ufunc also.

Can someone open an issue to discuss this?

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   fix passing of curvefit kwargs 1359368857
1256225233 https://github.com/pydata/xarray/pull/6978#issuecomment-1256225233 https://api.github.com/repos/pydata/xarray/issues/6978 IC_kwDOAMm_X85K4HnR slevang 39069044 2022-09-23T13:38:04Z 2022-09-23T13:38:04Z CONTRIBUTOR

Is that something that will be deprecated or is it planned to keep the support for the kwargs dict forever?

Not sure if there are any strong opinions here? I don't see much harm in keeping it around but we could also deprecate.

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   fix passing of curvefit kwargs 1359368857
1236072618 https://github.com/pydata/xarray/pull/6978#issuecomment-1236072618 https://api.github.com/repos/pydata/xarray/issues/6978 IC_kwDOAMm_X85JrPiq headtr1ck 43316012 2022-09-03T08:09:53Z 2022-09-11T07:16:20Z COLLABORATOR

Is that something that will be deprecated or is it planned to keep the support for the kwargs dict forever?

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   fix passing of curvefit kwargs 1359368857
1235974050 https://github.com/pydata/xarray/pull/6978#issuecomment-1235974050 https://api.github.com/repos/pydata/xarray/issues/6978 IC_kwDOAMm_X85Jq3ei slevang 39069044 2022-09-02T23:23:51Z 2022-09-02T23:23:51Z CONTRIBUTOR

Thanks for doing this @slevang ! Would you mind adding a tiny regression test too?

Right, I guess this actually breaks the previous way of passing kwargs and that is why the docs build failed. Hmmm. To go with the current changes, thoughts on adding something like this to the parsing logic: python if kwargs is None: kwargs = {} elif "kwargs" in kwargs: kwargs = {**kwargs.pop("kwargs"), **kwargs} to allow for the desired functionality but also handle the old case when someone passes kwargs as a dict? Feels wonky but it works. Is there a smarter way of doing this?

BTW it took me a minute to figure out what happened here because the docstring in the original PR was actually correct (requiring a dict, albeit maybe not the best way of passing kwargs), but then got changed in #5182 to suggest that kwargs could be passed on their own. I see .interp() behaves the same, where only a dict of kwargs can be provided and passed to the underlying scipy interpolation. So it could be worth generalizing this for other methods where we are passing kwargs to a child function of some sort, to allow both of these patterns.

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   fix passing of curvefit kwargs 1359368857
1235932096 https://github.com/pydata/xarray/pull/6978#issuecomment-1235932096 https://api.github.com/repos/pydata/xarray/issues/6978 IC_kwDOAMm_X85JqtPA TomNicholas 35968931 2022-09-02T22:12:37Z 2022-09-02T22:12:37Z MEMBER

Thanks for doing this @slevang ! Would you mind adding a tiny regression test too?

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   fix passing of curvefit kwargs 1359368857

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