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.

Sign up for GitHub

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

Jump to bottom

Fix static typing with Matplotlib 3.8 #8030

Merged
merged 25 commits into from
Sep 17, 2023
Merged

Fix static typing with Matplotlib 3.8 #8030

Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
fd2c467
fix several typing issues with mpl3.8
headtr1ck Jul 29, 2023
a97d634
Merge branch 'main' into pr/8030
Illviljan Aug 3, 2023
63409a5
Merge branch 'main' into mpltypes
headtr1ck Sep 5, 2023
3b62890
fix scatter plot typing and remove funky pyplot import
headtr1ck Sep 5, 2023
7b44b1b
fix some more typing errors
headtr1ck Sep 5, 2023
9051f30
fix some import errors
headtr1ck Sep 7, 2023
55a63b6
undo some typing errors
headtr1ck Sep 7, 2023
9e2c352
fix xylim typing
headtr1ck Sep 7, 2023
42d3356
add forgotten import
headtr1ck Sep 11, 2023
b5d2249
ignore plotting overloads because None is Hashable
headtr1ck Sep 11, 2023
d1cc216
add whats-new entry
headtr1ck Sep 11, 2023
428a1ed
fix return type of hist
headtr1ck Sep 11, 2023
2ab1402
fix another xylim type
headtr1ck Sep 11, 2023
db0db64
fix some more xylim types
headtr1ck Sep 11, 2023
3809152
Merge branch 'main' into mpltypes
headtr1ck Sep 11, 2023
8897ba1
change to T_DataArray
headtr1ck Sep 12, 2023
c318096
Merge branch 'main' into mpltypes
headtr1ck Sep 12, 2023
64c2401
change accessor xylim to tuple
headtr1ck Sep 12, 2023
79a92f5
add missing return types
headtr1ck Sep 12, 2023
b9dcc6a
fix a typing error only on new mpl
headtr1ck Sep 12, 2023
1eef361
add unused-ignore to error codes for old mpl
headtr1ck Sep 12, 2023
f7246fe
add more unused-ignore to error codes for old mpl
headtr1ck Sep 12, 2023
33c6f20
replace type: ignore[attr-defined] with assert hasattr
headtr1ck Sep 12, 2023
06e4337
Merge branch 'main' into mpltypes
headtr1ck Sep 16, 2023
b1d721c
apply code review suggestions
headtr1ck Sep 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions doc/whats-new.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,9 @@ Breaking changes
extracts and add the indexes from another :py:class:`Coordinates` object
passed via ``coords`` (:pull:`8107`).
By `Benoît Bovy <https://github.com/benbovy>`_.
- Static typing of ``xlim`` and ``ylim`` arguments in plotting functions now must
be ``tuple[float, float]`` to align with matplotlib requirements. (:issue:`7802`, :pull:`8030`).
By `Michael Niklas <https://github.com/headtr1ck>`_.

Deprecations
~~~~~~~~~~~~
Expand Down
10 changes: 4 additions & 6 deletions xarray/core/options.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,8 @@
from xarray.core.utils import FrozenDict

if TYPE_CHECKING:
try:
from matplotlib.colors import Colormap
except ImportError:
Colormap = str
from matplotlib.colors import Colormap

Options = Literal[
"arithmetic_join",
"cmap_divergent",
Expand Down Expand Up @@ -164,11 +162,11 @@ class set_options:
cmap_divergent : str or matplotlib.colors.Colormap, default: "RdBu_r"
Colormap to use for divergent data plots. If string, must be
matplotlib built-in colormap. Can also be a Colormap object
(e.g. mpl.cm.magma)
(e.g. mpl.colormaps["magma"])
cmap_sequential : str or matplotlib.colors.Colormap, default: "viridis"
Colormap to use for nondivergent data plots. If string, must be
matplotlib built-in colormap. Can also be a Colormap object
(e.g. mpl.cm.magma)
(e.g. mpl.colormaps["magma"])
display_expand_attrs : {"default", True, False}
Whether to expand the attributes section for display of
``DataArray`` or ``Dataset`` objects. Can be
Expand Down
21 changes: 12 additions & 9 deletions xarray/plot/accessor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
from matplotlib.container import BarContainer
from matplotlib.contour import QuadContourSet
from matplotlib.image import AxesImage
from matplotlib.patches import Polygon
from matplotlib.quiver import Quiver
from mpl_toolkits.mplot3d.art3d import Line3D, Poly3DCollection
from numpy.typing import ArrayLike
Expand Down Expand Up @@ -47,7 +48,9 @@ def __call__(self, **kwargs) -> Any:
return dataarray_plot.plot(self._da, **kwargs)

@functools.wraps(dataarray_plot.hist)
def hist(self, *args, **kwargs) -> tuple[np.ndarray, np.ndarray, BarContainer]:
def hist(
self, *args, **kwargs
) -> tuple[np.ndarray, np.ndarray, BarContainer | Polygon]:
return dataarray_plot.hist(self._da, *args, **kwargs)

@overload
Expand Down Expand Up @@ -179,7 +182,7 @@ def step(self, *args, **kwargs) -> list[Line3D] | FacetGrid[DataArray]:
return dataarray_plot.step(self._da, *args, **kwargs)

@overload
def scatter(
def scatter( # type: ignore[misc] # None is hashable :(
Copy link
Contributor

@Illviljan Illviljan Sep 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Have you tried Optional[Hashable] ?

I saw in the NamedArray PR we sometimes use this trick:

from xarray.core.utils import Default, _default

def _as_sparse(
        self: T_NamedArray,
        sparse_format: str | Default = _default,
        fill_value=dtypes.NA,
    ) -> T_NamedArray:


ef _replace(
        self: T_NamedArray, dims=_default, data=_default, attrs=_default
    ) -> T_NamedArray:
        if dims is _default:
            dims = copy.copy(self._dims)
        if data is _default:
            data = copy.copy(self._data)
        if attrs is _default:
            attrs = copy.copy(self._attrs)
        return type(self)(dims, data, attrs)

I am not sure how scary it would be to just change to this method?

Copy link
Collaborator Author

@headtr1ck headtr1ck Sep 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I saw in the NamedArray PR we sometimes use this trick:
I am not sure how scary it would be to just change to this method?

I don't think one should deviate from None as default if it is preventable.

This issue if exactly this: python/mypy#13805
Not sure why for some methods it is only surfacing now and not already before.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Have you tried Optional[Hashable] ?

That is exactly the same as Hashable | None, and tools like pyupgrade will also replace that.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess the question is; does xarray allow None as a dim/coordinate? If we do, we have to use other defaults.

Considering our insistence to use Hashable for dims it seems we want to:

a = xr.Variable(data=[3,4], dims=(None,))
print(a)
<xarray.Variable (None: 2)>
array([3, 4])

c = xr.DataArray([1,2]).assign_coords(coords={"dim_0": [3,4]}).rename({"dim_0": None})
print(c)
<xarray.DataArray (None: 2)>
array([1, 2])
Coordinates:
  * None     (None) int32 3 4

I suppose this is just another issue with dim(s) haven't been consistently typed before.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

While None is technically hashable probably half of xarrays functions use it as default for dimension arguments, so it would be quite a large breaking change to allow it as a dimension/variable/coordinate "name".

I think it is not worth it to introduce such a breaking change just to allow such a corner case.
If something, we should probably add a check for not None in the constructors of xarray objects.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If you want, you can open a follow up issue for that.
Definitely this is out of scope for this PR.

self,
*args: Any,
x: Hashable | None = None,
Expand Down Expand Up @@ -306,7 +309,7 @@ def scatter(self, *args, **kwargs):
return dataarray_plot.scatter(self._da, *args, **kwargs)

@overload
def imshow(
def imshow( # type: ignore[misc] # None is hashable :(
self,
*args: Any,
x: Hashable | None = None,
Expand Down Expand Up @@ -430,7 +433,7 @@ def imshow(self, *args, **kwargs) -> AxesImage:
return dataarray_plot.imshow(self._da, *args, **kwargs)

@overload
def contour(
def contour( # type: ignore[misc] # None is hashable :(
self,
*args: Any,
x: Hashable | None = None,
Expand Down Expand Up @@ -554,7 +557,7 @@ def contour(self, *args, **kwargs) -> QuadContourSet:
return dataarray_plot.contour(self._da, *args, **kwargs)

@overload
def contourf(
def contourf( # type: ignore[misc] # None is hashable :(
self,
*args: Any,
x: Hashable | None = None,
Expand Down Expand Up @@ -678,7 +681,7 @@ def contourf(self, *args, **kwargs) -> QuadContourSet:
return dataarray_plot.contourf(self._da, *args, **kwargs)

@overload
def pcolormesh(
def pcolormesh( # type: ignore[misc] # None is hashable :(
self,
*args: Any,
x: Hashable | None = None,
Expand Down Expand Up @@ -945,7 +948,7 @@ def __call__(self, *args, **kwargs) -> NoReturn:
)

@overload
def scatter(
def scatter( # type: ignore[misc] # None is hashable :(
self,
*args: Any,
x: Hashable | None = None,
Expand Down Expand Up @@ -1072,7 +1075,7 @@ def scatter(self, *args, **kwargs) -> PathCollection | FacetGrid[DataArray]:
return dataset_plot.scatter(self._ds, *args, **kwargs)

@overload
def quiver(
def quiver( # type: ignore[misc] # None is hashable :(
self,
*args: Any,
x: Hashable | None = None,
Expand Down Expand Up @@ -1187,7 +1190,7 @@ def quiver(self, *args, **kwargs) -> Quiver | FacetGrid:
return dataset_plot.quiver(self._ds, *args, **kwargs)

@overload
def streamplot(
def streamplot( # type: ignore[misc] # None is hashable :(
self,
*args: Any,
x: Hashable | None = None,
Expand Down
Loading
Loading