Fix static typing with Matplotlib 3.8 #8030
Conversation
headtr1ck
commented
Jul 29, 2023
headtr1ck
commented
- Closes Mypy errors with matplotlib 3.8 #7802
- Tests added
|
Any Idea how I can run mypy vs. upstream? |
|
The label |
Oh cool, thanks!
I'll have to finish this after the holidays :) |
|
It looks intimidating, but its actually only a small number of (similar) tweaks to solve most of the flags. The biggest/most complicated ones are the overloaded signatures (which I don't think are directly matplotlib, just perhaps its able to check them a bit more thoroughly now.) The core is that I think most of the remaining mpl ones are "real" from our perspective, though I do have a patch that I'm preparing to PR on our end for a good number of the utils.py:1349 issues at least. More detailed analysis/breakdownA diff that may help, but not sure its precisely what you wantDoes a couple things:
diff --git a/xarray/plot/dataarray_plot.py b/xarray/plot/dataarray_plot.py
index 4318b3db..ce8c4e87 100644
--- a/xarray/plot/dataarray_plot.py
+++ b/xarray/plot/dataarray_plot.py
@@ -34,6 +34,8 @@ from xarray.plot.utils import (
)
if TYPE_CHECKING:
+ from typing import ParamSpec, TypeVar
+
from matplotlib.axes import Axes
from matplotlib.collections import PathCollection, QuadMesh
from matplotlib.colors import Colormap, Normalize
@@ -53,6 +55,9 @@ if TYPE_CHECKING:
)
from xarray.plot.facetgrid import FacetGrid
+ P = ParamSpec("P")
+ R = TypeVar("R")
+
_styles: MutableMapping[str, Any] = {
# Add a white border to make it easier seeing overlapping markers:
"scatter.edgecolors": "w",
@@ -718,7 +723,7 @@ def hist(
return primitive
-def _plot1d(plotfunc):
+def _plot1d(plotfunc: Callable[P, R]) -> Callable[P, R | FacetGrid[DataArray]]:
"""Decorator for common 1d plotting logic."""
commondoc = """
Parameters
@@ -1075,9 +1080,9 @@ def _plot1d(plotfunc):
# we want to actually expose the signature of newplotfunc
# and not the copied **kwargs from the plotfunc which
# and not the copied **kwargs from the plotfunc which
from matplotlib.axes import Axes
from matplotlib.collections import PathCollection, QuadMesh
from matplotlib.colors import Colormap, Normalize
@@ -1238,8 +1243,8 @@ def scatter(
@_plot1d
def scatter(
- xplt: DataArray | None,
- yplt: DataArray | None,
+ xplt: DataArray,
+ yplt: DataArray,
ax: Axes,
add_labels: bool | Iterable[bool] = True,
**kwargs,
@@ -1261,13 +1266,15 @@ def scatter(
if sizeplt is not None:
kwargs.update(s=sizeplt.to_numpy().ravel())
- axis_order = ["x", "y", "z"]
-
- plts_dict: dict[str, DataArray | None] = dict(x=xplt, y=yplt, z=zplt)
- plts_or_none = [plts_dict[v] for v in axis_order]
- plts = [p.to_numpy().ravel() for p in plts_or_none if p is not None]
- primitive = ax.scatter(*plts, **kwargs)
- _add_labels(add_labels, plts, ("", "", ""), (True, False, False), ax)
+ xplt_np = xplt.to_numpy().ravel()
+ yplt_np = yplt.to_numpy().ravel()
+ if zplt is None:
+ primitive = ax.scatter(xplt_np, yplt_np, **kwargs)
+ _add_labels(add_labels, [xplt, yplt], ("", "", ""), (True, False, False), ax)
+ else:
+ zplt_np = zplt.to_numpy().ravel()
+ primitive = ax.scatter(xplt_np, yplt_np, zplt_np, **kwargs)
+ _add_labels(add_labels, [xplt, yplt], ("", "", ""), (True, False, False), ax)
return primitive
diff --git a/xarray/plot/dataset_plot.py b/xarray/plot/dataset_plot.py
index b0774c31..7a70f1d4 100644
--- a/xarray/plot/dataset_plot.py
+++ b/xarray/plot/dataset_plot.py
@@ -632,7 +632,7 @@ def streamplot(
du = du.transpose(ydim, xdim)
dv = dv.transpose(ydim, xdim)
- args = [dx.values, dy.values, du.values, dv.values]
+ args = (dx.values, dy.values, du.values, dv.values)
hue = kwargs.pop("hue")
cmap_params = kwargs.pop("cmap_params")
|
xref pydata/xarray#8030 We were perhaps overly permissive in the return type, I'm pretty sure we always return Masked Array, so may as well say so, so that downstream can rely on that (which they already are, to be clear...)
|
@ksunden thanks for the detailed breakdown. I'm currently on holiday (mobile only) so I will not continue this for another three weeks. If you or someone else is motivated in the meantime, feel free to continue where I left. Just some remarks: We didn't use ParamSpec so far because it is python 3.10 only, but I guess typing_extensions should have it as well. (In that regard, you should check if your type annotations work on the oldest python version that mpl supports, we had some complaints about this in the past). Edit: actually we just started using Self which is python 3.11, so it should definitely work. I am not sure if restricting xlim to a length two tuple is a good idea. I guess it depends on what you want to achieve with type annotations. I assume many people will pass a list to this method and would get errors on code that actually runs just fine. If your goal is inline editor hints on what to pass, I agree that the tuple is more clear. |
|
WRT ParamSpec, we only use it a handful of times, and do import from typing_extensions when we do (and are guarded by WRT xlim tuples, I do wish there was a better way to say "length-2 sequence", but that is not supported by the current type system (and probably won't be). But when other length sequences will break, I still end up on the side of "more useful than harmful", but not as solidly as I once was, admittedly. xref python/mypy#8441 It's been proposed an thought of, but is low priority. |
|
@ksunden I'm finally continuing this PR here :) Would you mind changing the return type of It is difficult to work with |
Nvmd, that has happened already, just forgot to update my local mpl repo... |
|
@Illviljan Maybe you could do a quick review already and help with the remaining errors. CI Additional: Ci Upstream:
etc. The functools.wraps seems to not work anymore :/ |
|
@ksunden I think I have resolved all parts that are dependent on matplotlib. Thanks for the typing improvements! |
Unfortunately that is not super trivial because of how we have some generated code for Not impossible, but a bit larger of a task than it may seem at first. I don't think that the overloads would have saved you here, though, either. Also the list vs non-list outputs are potentially hard to differentiate because it is based on the shape rather than the value... ( We could potentially acquiesce that that return type is more trouble than it is worth and just fall back to Feel free to open an issue, I don't think it will make it into 3.8.0 (which I'd like to get out asap) |
There was a problem hiding this comment.
You probably saw this thread as well but have you tried changing the order of the overloads?
python/mypy#5407
xarray/plot/accessor.py
Outdated
| @@ -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 :( | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Have you tried Optional[Hashable] ?
That is exactly the same as Hashable | None, and tools like pyupgrade will also replace that.
There was a problem hiding this comment.
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 4I suppose this is just another issue with dim(s) haven't been consistently typed before.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
If you want, you can open a follow up issue for that.
Definitely this is out of scope for this PR.
Yes, I tried but same result. |
|
Current Mypy is successful, upstream mypy only shows numpy errors. |
|
@ksunden thanks for the help! |
* fix several typing issues with mpl3.8 * fix scatter plot typing and remove funky pyplot import * fix some more typing errors * fix some import errors * undo some typing errors * fix xylim typing * add forgotten import * ignore plotting overloads because None is Hashable * add whats-new entry * fix return type of hist * fix another xylim type * fix some more xylim types * change to T_DataArray * change accessor xylim to tuple * add missing return types * fix a typing error only on new mpl * add unused-ignore to error codes for old mpl * add more unused-ignore to error codes for old mpl * replace type: ignore[attr-defined] with assert hasattr * apply code review suggestions --------- Co-authored-by: Illviljan <14371165+Illviljan@users.noreply.github.com>
