Skip to content

Type checking fails for multiplication #4054

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

Closed
jenssss opened this issue May 13, 2020 · 6 comments · Fixed by #4904
Closed

Type checking fails for multiplication #4054

jenssss opened this issue May 13, 2020 · 6 comments · Fixed by #4904

Comments

@jenssss
Copy link
Contributor

jenssss commented May 13, 2020

MCVE Code Sample

I save the following in a file called "foo.py"

import xarray as xr

def mul(x: float, y: xr.Dataset):
    return x*y

and run

mypy foo.py

Expected Output

This should give

Success: no issues found in 1 source file

Problem Description

Instead I get the output

foo.py:4: error: Unsupported operand types for * ("float" and "Dataset")

It gives a similar error for addition.
If I reverse the order of multiplication it works as expected, namely

import xarray as xr

def mul(x: float, y: xr.Dataset):
    return y*x

gives

Success: no issues found in 1 source file

But it would be nice if it also worked when the float is first, so I don't have to change a lot existing code to get typechecking to work.

Versions

I tested on version 0.15.1 and on the current master

Output of xr.show_versions()

INSTALLED VERSIONS

commit: None
python: 3.8.2 (default, May 7 2020, 20:00:49)
[GCC 7.3.0]
python-bits: 64
OS: Linux
OS-release: 5.4.0-29-generic
machine: x86_64
processor: x86_64
byteorder: little
LC_ALL: None
LANG: en_US.UTF-8
LOCALE: en_US.UTF-8
libhdf5: None
libnetcdf: None

xarray: 0.15.2.dev61+gbd84186a
pandas: 1.0.3
numpy: 1.18.4
scipy: None
netCDF4: None
pydap: None
h5netcdf: None
h5py: None
Nio: None
zarr: None
cftime: None
nc_time_axis: None
PseudoNetCDF: None
rasterio: None
cfgrib: None
iris: None
bottleneck: None
dask: None
distributed: None
matplotlib: None
cartopy: None
seaborn: None
numbagg: None
pint: None
setuptools: 46.2.0.post20200511
pip: 20.0.2
conda: None
pytest: None
IPython: None
sphinx: None

@Zeitsperre
Copy link
Contributor

Zeitsperre commented Aug 20, 2020

We're currently working on a library largely based on xarray and have seen the same types of errors from mypy (PR in our project that is currently trying to integrate mypy: Ouranosinc/xclim#532). Currently working off of xarray v0.16.

I also want to note this error is raised for other operations as well (+, -, /, and *) between xarray.DataArray and xarray.Datasets.

@max-sixty
Copy link
Collaborator

Yes, this looks like incomplete typing.

We would take a PR for these for sure.

@dcherian
Copy link
Contributor

Yeah there's no type info in DataArray._binary_op, Variable._binary_op and Dataset._binary_op

@Zeitsperre
Copy link
Contributor

Just dicsovered that the same things is true for ~. Another thing to add to the list:
... : error: Unsupported operand type for ~ ("DataArray") [operator]

@max-sixty
Copy link
Collaborator

Given these are generated dynamically, is there a clever way to define types for these in Python? Or we have to write out all the methods?

https://github.com/pydata/xarray/blob/v0.16.0/xarray/core/ops.py#L312-L316

@rhkleijn
Copy link
Contributor

I have implemented a possible solution to have typing for all unary and binary operators which works for mypy and also displays correctly inferred type hints on hovering over the code in Pylance (with VS Code) and PyCharm. The approach involves generating a stub file using a simple helper script. I'll open a draft PR so you can see this approach and for soliciting feedback.

I have been experimenting with all kinds of other approaches to solve this issue but this was the only one with a satisfactory result. For reference, and jotting down from memory, I think I tried the following approaches without much success:

  • add the typing information to the not_implemented function in
    # this has no runtime function - these are listed so IDEs know these
    # methods are defined and don't warn on these operations
    __lt__ = (
    __le__
    ) = (
    __ge__
    ) = (
    __gt__
    ) = (
    __add__
    ) = (
    __sub__
    ) = (
    __mul__
    ) = (
    __truediv__
    ) = (
    __floordiv__
    ) = (
    __mod__
    ) = (
    __pow__
    ) = __and__ = __xor__ = __or__ = __div__ = __eq__ = __ne__ = not_implemented
  • adding type declarations in a Generic mixin class parameterized on the return type of the methods. This doesn't handle dependencies of the return type on the type of the second argument of binary operators.
  • adding type declarations in a Generic mixin class parameterized on a Callback Protocol (PEP 544). This is too much
    magic for typing tools to grasp.
  • adding type declarations directly using a Callback Protocol. This seems to works fine for regular function but for methods the type of self of the class implementing the Protocol gets confused with the type of self in the Protocol itself.
  • using an overloaded abstract method for one operator (and then assigning the method to the dunders of all other operators to avoid a lot of repetition) results in mypy errors because it does not recognize that the concrete implementations are added dynamically. Making them concrete and returning NotImplemented doesn't work as type checkers are not sufficiently aware of the fallback logic of calling the corresponding reflexive method on the other argument of the binary operation. And raising causes some type checkers to think NoReturn is the intended return type.

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

Successfully merging a pull request may close this issue.

5 participants