xarray icon indicating copy to clipboard operation
xarray copied to clipboard

core.missing typing

Open hollymandel opened this issue 1 year ago • 4 comments

I have not finished the following yet. I am posting this as a draft PR to solicit feedback about typing philosophy/style, in particular about the balance between permissive typing (Union[...]) versus overloading versus substantive code changes.

  • [ ] Adding type hints + mypy comments to missing.py
  • [ ] Code-affecting changes to core/missing.py module to create type consistency
  • [ ] Adding/editing typing comments of modules called by missing.py (as guided by mypy) but no code-affecting changes
  • [ ] Adding Pandas to mypy ignore

hollymandel avatar Oct 05 '24 00:10 hollymandel

Lots of type hints are np.ndarray, have you checked if this is indeed the only allowed argument and no Duckarrays or > Thank you for this PR. This topic is a complicated endeavor, trying to type such deeply internal stuff.

Unfortunately I think some more work had to be put into it. The pandas stubs are just one part. Lots of type hints are np.ndarray, have you checked if this is indeed the only allowed argument and no Duckarrays or dask Arrays etc. are allowed?

Thanks for the detailed feedback! I think I have a big-picture question about how typing is intended to develop. I was viewing this PR as a delta towards there being more documentation of this module, subject to maybe the behaviors in the documentation and testing modules. However if type hints are going to be viewed as a contract to which future users/developers will ascribe a high degree of credence, or if adding them can break things in some way that I'm not aware of, I'm not in a position to provide that.

hollymandel avatar Oct 08 '24 17:10 hollymandel

I didn't want to discourage you, we are able to help and guide you in this PR. I'm currently on mobile only, so it might take some weeks from my side.

Unfortunately many developers are not super familiar with static typing and the past has shown that there are rarely any changes on existing type hints on internal stuff. This means we should try to get it as right as possible on the first try, otherwise it will lead to issues later on when people try to adhere to the type hints not knowing that the method actually can do more (e.g. support dask Arrays when the type hints says np.ndarray only).

If any other developer disagrees with that statement, please say so.

So again, we appreciate this PR and are willing to support you to get this in shape.

headtr1ck avatar Oct 09 '24 08:10 headtr1ck

Thanks, that makes sense. Appreciate the support and feedback!

hollymandel avatar Oct 09 '24 18:10 hollymandel

@headtr1ck Thanks again for your help with this. I had two additional questions.

  1. I'm trying to use DuckArrays (T_DuckArray) when only DuckArray methods are required for the function to run, however it seems that python rejects covariant types as function parameters for functions that are not methods I guess due to dangers of corrupting any resulting generic types. So I'm not sure the best way to indicate that Duck Arrays are supported.

  2. There are also some instances where Vairable, DataArray, and Dataset are all supported but not more numpy-like arrays, can't find an already-defined class that describes this behavior. I guess the shared behavior has something to do with support for indexing?

hollymandel avatar Oct 15 '24 18:10 hollymandel