home / github / issue_comments

Menu
  • Search all tables
  • GraphQL API

issue_comments: 798793062

This data as json

html_url issue_url id node_id user created_at updated_at author_association body reactions performed_via_github_app issue
https://github.com/pydata/xarray/pull/4904#issuecomment-798793062 https://api.github.com/repos/pydata/xarray/issues/4904 798793062 MDEyOklzc3VlQ29tbWVudDc5ODc5MzA2Mg== 32801740 2021-03-13T22:07:41Z 2021-03-13T22:07:41Z CONTRIBUTOR

The gen-ops branch looks really cool @rhkleijn .

thanks!

I have a few questions, but they don't need to hold back merging — if others agree — in particular on the .py and .pyi design — we could merge now and then iterate.

  • Above you mentioned: The mypy documentation states that if a directory contains both a .py and a .pyi file for the same module, the .pyi file takes precedence. It is my understanding that stub files work on a file by file basis and you cannot mix them for a single file.

    And here we have both a xarray/core/_typed_ops.pyi and xarray/core/_typed_ops.py. Does that mean the .pyi file is obscuring the .py file?

It takes precedence only during type checking as the source for static type information. The generated .py and .pyi files are by their construction in sync and since the .py file does not contain typing information nothing gets obscured here. At runtime the .pyi file is ignored.

  • Do you know the extent to which PyCharm not picking up the @overloads is because PyCharm hasn't implemented a PEP yet, vs. an issue with the proposed code? If PyCharm is going to catch up soon, I don't think we need to engineer around it.

There seem to be quite some open issues here with no indication they currently are actively working on fixing them. I am not using PyCharm myself, but as it is widely used I would also expect quite some xarray users to be using it.

  • Python typing has run ahead of me recently, so forgive the uninformed question, but do you know whether it's possible to define a parent class to be generic over a type, and avoid repeating methods like this for each Mixin class? python def __neg__(self: T_Dataset) -> T_Dataset: ... def __pos__(self: T_DataArray) -> T_DataArray: ... def __neg__(self: T_Variable) -> T_Variable: ... Or are there only a few of these that are common across the classes, such that it's not worth having a parent class for the Mixins?

For these unary ops with simple type hints if would probably work but for most other methods with multiple overloads the typing ecosystem is not yet mature enough. In this issue I have commented on it. I hope one day we will be able to refactor this into some generic parent classes (one for unary ops, one for binary (and reflexive) ops and one for inplace ops) from which we can inherit. Using a generic parent class only for the unary ops and not for the others would make the script more complicated. For now, I'd say this somewhat low-tech solution gets the job done.

  • Do we need to define methods for the DataArray's ops with a Dataset — e.g. here — or can we rely on Datasets implementation? I think that's how the runtime handles it.

Unfortunately we have to define them both ways for typing purposes. This comment in generate_ops.py contains some explanation: ```

Note: in some of the overloads below the return value in reality is NotImplemented,

which cannot accurately be expressed with type hints,e.g. Literal[NotImplemented]

or type(NotImplemented) are not allowed and NoReturn has a different meaning.

In such cases we are lending the type checkers a hand by specifying the return type

of the corresponding reflexive method on other which will be called instead.

``` Also here, my hope is that someday this will not be needed anymore.

  • Do we need to define these NoReturn methods? I would have thought that if there weren't a valid type signature we'd get an error anyway.

The NoReturn reflects the exception raised from the GroupBy._binary_op method here. It might help users trying to e.g. multiply a GroupBy object and a Variable. With NoReturn mypy correctly errors in that case. Without NoReturn mypy incorrectly infers the result to be Variable. It seems to be shortcoming of mypy, since Pylance does infer Any in that case (which doesn't rule out a NoReturn).

Using python to template the methods works pretty well! Better than I would have expected relative to a templating library like jinja.

I was also pleasantly surprised by it. And even black leaves the generated files unchanged.

{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
  807764700
Powered by Datasette · Queries took 0.728ms · About: xarray-datasette