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

Use definition of DTypeLike from Numpy if found #4941

Merged
merged 2 commits into from
Feb 23, 2021

Conversation

jenshnielsen
Copy link
Contributor

@jenshnielsen jenshnielsen commented Feb 22, 2021

This allows application of numpy ufuncs to DataArrays to typecheck correctly with Numpy 1.20

E.g. without this fix:

import xarray as xr
import numpy as np

def applyufunctodataarray() -> xr.DataArray:
    a = xr.DataArray(np.array([-1,2,3]))
    b = np.abs(a)
    return b

will result in:

test.py:9: error: Argument 1 to "__call__" of "ufunc" has incompatible type "DataArray"; expected "Union[Union[int, float, complex, str, bytes, generic], Sequence[Union[int, float, complex, str, bytes, generic]], Sequence[Sequence[Any]], _SupportsArray]"
Found 1 error in 1 file (checked 1 source file)

While it will type check without issues with this fix.

I have not yet documented this or added tests.
Please let me know if this change is acceptable and I am happy to do that.

  • Tests added
  • Passes pre-commit run --all-files
  • User visible changes (including notable bug fixes) are documented in whats-new.rst

And fall back to original definition with older versions of numpy
@pep8speaks
Copy link

pep8speaks commented Feb 22, 2021

Hello @jenshnielsen! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-02-22 08:09:19 UTC

Copy link
Collaborator

@max-sixty max-sixty left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @jenshnielsen , we're happy to have you as a contributor.

This looks good! We do have a mechanism for checking versions elsewhere in the code using LooseVersion, but this approach works too. IIUC the existing code may break type-checking, but the code will still run on numpy<1.20; please let me know if that's not the case.

I'm not that well-versed here, so I'll wait a day for merging to see whether others have comments.

@jenshnielsen
Copy link
Contributor Author

jenshnielsen commented Feb 23, 2021

@max-sixty Thanks :)

IIUC the existing code may break type-checking, but the code will still run on numpy<1.20; please let me know if that's not the case.

Yes the existing code which is now the fallback could be considered wrong as it does not match the definitions that numpy ships.
We could consider syncing that with numpy but that seems like more trouble than its worth since no one is likely to typecheck this against the definitions of numpy functions without running numpy 1.20 or newer.

The numpy definition is:

DTypeLike = Union[
    np.dtype,
    # default data type (float64)
    None,
    # array-scalar types and generic types
    type,  # TODO: enumerate these when we add type hints for numpy scalars
    # anything with a dtype attribute
    "_SupportsDType[np.dtype[Any]]",
    # character codes, type strings or comma-separated fields, e.g., 'float64'
    str,
    _VoidDTypeLike,
]

So one could add None and type but should probably omit the rest in order to not replicate too much of numpys internal type definitions

@mathause
Copy link
Collaborator

Thanks. Looks good. IMHO typing is a nice-to-have far-from-complete part of xarray & I wouldn't bother about backwards compatibility (of the typing). Up to you.

@max-sixty
Copy link
Collaborator

Great, merging. Thanks @jenshnielsen !

@max-sixty max-sixty merged commit 348eb48 into pydata:master Feb 23, 2021
@jenshnielsen jenshnielsen deleted the fix_typechecking_with_numpy_120 branch February 23, 2021 19:27
@jenshnielsen
Copy link
Contributor Author

Thanks @max-sixty @mathause

dcherian added a commit to dcherian/xarray that referenced this pull request Mar 8, 2021
* upstream/master: (46 commits)
  pin netCDF4=1.5.3 in min-all-deps (pydata#4982)
  fix matplotlib errors for single level discrete colormaps (pydata#4256)
  Adapt exception handling in CFTimeIndex.__sub__ and __rsub__ (pydata#5006)
  Update options.py (pydata#5000)
  Adjust tests to use updated pandas syntax for offsets (pydata#4537)
  add a combine_attrs parameter to Dataset.merge (pydata#4895)
  Support for dask.graph_manipulation (pydata#4965)
  raise on passing axis to Dataset.reduce methods (pydata#4940)
  Whatsnew for 0.17.1 (pydata#4963)
  Refinements to how-to-release (pydata#4964)
  DOC: add example for reindex (pydata#4956)
  DOC: rm np import (pydata#4949)
  Add 0.17.0 release notes (pydata#4953)
  document update as inplace (pydata#4932)
  bump the dependencies (pydata#4942)
  Upstream CI: limit runtime (pydata#4946)
  typing for numpy 1.20 (pydata#4878)
  Use definition of DTypeLike from Numpy if found (pydata#4941)
  autoupdate mypy (pydata#4943)
  Add DataArrayCoarsen.reduce and DatasetCoarsen.reduce methods (pydata#4939)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants