home / github / issue_comments

Menu
  • GraphQL API
  • Search all tables

issue_comments: 779404893

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-779404893 https://api.github.com/repos/pydata/xarray/issues/4904 779404893 MDEyOklzc3VlQ29tbWVudDc3OTQwNDg5Mw== 32801740 2021-02-15T19:10:56Z 2021-02-15T19:10:56Z CONTRIBUTOR

Thanks for the PR & your work. I'll need some time to digest that all... One thing I certainly would like to fix before merging this is #4881 (and merge #4878).

I agree. Both PRs you mention would be nice to see merged. I was still using numpy 1.19.

I'd also be interested in your motivation for the stub files. (It's just that we don't have any of these so far).

I usually prefer inline type hints. The motivation for stub files was mostly because all other approaches I tried where not sufficient, see my comment in #4054. It turns out we might not even need a stub file, see my reply above to max-sixty's comments.

I am sure you saw this comment: ... so there would be an argument to create mixin-classes*. Another motivation would be that currently (with the inject approach) mypy does not know which methods DataArray actually has. This is kind of hidden by __getattr__ (see #4601 & #4616). Now I am not saying you have to do this here but also wanted to mention it.

  • Well I just realised that your stub files are kind of mixin classes...

That is correct. It can currently be considered a mixin class during type checking and a no-op at compile and runtime. If I can get max-sixty's suggestion to work, then we would quite literally be following up on that TODO by using mixin classes instead of the unintuitive "inject" functions.

What are all the # type: ignore[misc] you are suppressing?

Those are mypy warning for overlapping overload signatures with incompatible return types. In practice it is not a problem as the first matching overload gets picked. One reason for overlapping is that with numpy <=1.20 numpy.ndarray is typed as Any, which makes it overlap with other overload signatures. Another one is due to mypy checking the consistency of binops and their reflexive counterparts.

I was slightly confused by the T_ of T_Other etc because this is not a TypeVar (but an alias) so I'd prefer another name.

Agreed. What name would work in this case? Something like OtherType or just Other (or ScalarOrArray)? Anything else?

For T_Dataset = TypeVar("T_Dataset", bound=Dataset) etc - why do you use a bound? To allow subclassing? As we discourage subclassing we may have to discuss if we should allow this.

I think it would be nice to allow some simple kinds of subclassing (some (many?) methods already preserve the subclass in the return value but the type hint mentions the base class as return type). The particular use case I have in mind involves having a custom Accessor registered with xr.register_dataarray_accessor. Currently I have found no way to add meaningful type hints for these dynamically added attributes on the DataArray or Dataset classes, and I always wish I had signature information and type hints available when working on code using the accessor. One way this could be achieved is by using a simple wrapper class during type checking which includes a type declaration to signal both the existence and the type of the accessor, e.g. something along the lines of

``` import xarray as xr

if TYPE_CHECKING: class Dataset(xr.Dataset): myaccessor: MyAccessor else: Dataset = xr.Dataset # note that at runtime we just work with xr.Dataset

ds = Dataset(...) ds2 = (ds + 1).sel(x=1) # some operations (ideally preserving subclass!)

In the line below I would like my IDE to be able to show me the docstrings,

methods signatures and typing information for ds2.myaccessor and for

ds2.myaccessor.method and the inferred type of result

result = ds2.myaccessor.method()
```

I will also bring this use case up in #3980. Until that is resolved, I am open to here use either bound TypeVars or just "Dataset" and "DataArray".

{
    "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.818ms · About: xarray-datasette