Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core.missing typing #9584

Draft
wants to merge 13 commits into
base: main
Choose a base branch
from
Draft

Conversation

hollymandel
Copy link
Contributor

@hollymandel hollymandel commented Oct 5, 2024

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 hollymandel changed the title Interp typing (Draft PR) core.missing typing Oct 5, 2024
xarray/core/dataset.py Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
Copy link
Collaborator

@headtr1ck headtr1ck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

xarray/core/missing.py Outdated Show resolved Hide resolved
nu=0,
ext=None,
xi: Variable,
yi: np.ndarray,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure that xi and yi have different types?
Im on mobile only, so cannot really check this.

xarray/core/missing.py Outdated Show resolved Hide resolved
@@ -696,7 +706,7 @@ def interp_func(var, x, new_x, method: InterpOptions, kwargs):
scipy.interpolate.interp1d
"""
if not x:
return var.copy()
return var.data.copy()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to go beyond type hints. Are you sure that this is correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think yes though I'm not sure what level of certainty is appropriate. The output is passed as data to the Variable constructor which just wants to treat it as a numpy-like array.

@hollymandel
Copy link
Contributor Author

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.

@headtr1ck
Copy link
Collaborator

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.

@hollymandel
Copy link
Contributor Author

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

@hollymandel
Copy link
Contributor Author

hollymandel commented Oct 15, 2024

@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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants