issue_comments

I get much more overlap than you! Why is that? Windows again?

I don't necessarily agree about y-axis pointing upwards, I think that is confusing. But I let people who actually use this be the judge :)

For me this code works? Besides from too few distance between the subplots such that the z-labels are on top of the neighboring plot. And the y and z axis seem to be reversed, unless it is intentional that y points to the top?

Ok, good question. I have to look into it. Anyway, you should use kwargs for x and y as well :) Also, we should definitely test this haha

Next time I should split such a PR into smaller ones, haha. Anyone ready for a final review?

That was quite a merge, haha.... @Illviljan I had to undo some of your work, it just did not work well with static typing. Hopefully you are not too sad about it. :)

Is there a way to see the generated docs to check if my "hacks" worked?

Excellent! Should we hit the green button?

Don't know if @Illviljan wants to merge his PR first or prefers to solve the merge conflicts himself :P

So finally I have decided to properly deprecate positional arguments since I think many users are still using them (even many tests where using them...)

wait.... did mypy 0.981 change the behavior of overloads with None and Hashable? I now get errors for ```python from typing import Hashable

@overload def x(a: Hashable) -> Hashable: ...

@overload def x(a: None = None) -> None: ...

def x(a: Hashable | None = None) -> Hashable | None: return a ``` but this works with 0.971. I cannot find anything on the changelog... Interesting that does not break other things in xarray since None is always treated differently! (In theory mypy is right, since None is actually hashable...)

Edit: see https://github.com/python/mypy/issues/13805, adding some type: ignore for this is the "solution"

One point aside — when this is done, we may be able to turn on mypy's strict mode, at least for the tests path, so then tests always need annotations, and our annotations get tested.

(for context, a year ago or so we found some of them were wrong, but weren't being tested, because -> None wasn't in the tests)

I think there are still many untyped areas, especially internal stuff. But we can give it a try and see where it collapses :)

Finally I am happy with the changes, the signatures are correct at runtime and static type checking time. I was finally: 1. renaming the xarray.plot.plot module to xarray.plot.dataarray_plot since it was shadowing the plot method (Is added to breaking changes in whats-new, hopefully not many people are importing from this module directly) 2. creating a new xarray.plot.accessor module (see https://github.com/pydata/xarray/pull/7052#issuecomment-1257029973 option-1)

@Illviljan I'm sorry if you have to change your plotting PRs if this gets merged first. If you prefer, we can merge your PRs first and I will fix the conflicts here :)

I have tried to solve it with ParamSpec and writing a custom wraps decorator, but it did not work, presumably due to https://github.com/python/mypy/issues/13540

Maybe there's something to learn from how pandas does it: https://github.com/pandas-dev/pandas/tree/main/pandas/plotting ?

Just had a quick look and I think their typing of the accessor is wrong, haha. They claim to return a PlotAccessor instance when calling e.g. df.plot.line(). But I did not test it. Edit: turns out that it works, but I have no idea how or why, haha Edit2: only pyright can resolve the type of these function, mypy says Any...

I'm a little skeptical if all the arguments in scatter are necessary and maybe they can be hidden in kwargs? Should make the overloads a little shorter.

I've been working on moving all plots to the DataArray side starting with scatter in #6778. It should also remove the list[PathCollection] overloads.

I saw that, that's why I left the DataArray scatter untouched for now.

For now I see two options:

1. Move the accessor to its own module. For the user nothing should change since nobody should use the accessor directly anyway.
2. Create a plot.pyi typing file (should hopefully also resolve the wrapping issue since the overload only lives there)

In principle also a combination of the two is possible :)

Ok found another interesting problem: the e.g. functools.wraps(imshow) in the PlotAccessor works because in the moment it is called, there is no class method named "imshow", so it will use the module method, which is exactly what we want. However, as soon as one adds overloads, there indeed is a "imshow" class method (just the overload wrapper) and it tries to wrap this instead, which goes wrong (so the overload shadows the module method that we want)...

Does anyone know how we can force it to use the module imshow instead of the overload wrapper? The only thing I can think of is to move the Accessor to its own module, which might actually not be a bad idea?

Wow, to fix the overloads I had to copy-paste the complete function signature for each overload and again for the accessor methods. This leads to a massive overhead which I assume the initial author wanted to prevent by using the decorators in the first place...

Are there any objections to leaving it like this? We could also deviate from the approach of adding typing inline and use stub files (.pyi like in typed_ops).

Finally it is a much larger PR than initially expected. Turns out that the col and row arguments have to be overloaded since they return a FacetGrid instead of a primitive.

I am now stuck in trying to resolve the broken test. Does anyone know how to use @overload decorators on a function that is wrapped with functools.wraps(...)? This does not seem to work and the signature is messed up?

The 2D plotfunctions have the correct annotions at runtime (plotfunc.__annotations__) but mypy thinks they are args, *kwargs. Same with the accessor methods.

Does anyone have an idea how to tell mypy that the type is actually the function signature of the "inside" newplotmethod function? Do we need to define a dummy method with the same signature or something?

I could add some return type annotations, but I doubt that mypy can work with the signature hacks and will always use args, *kwargs.

Any thoughts?

Puh, this stacking of decorators is quite a brainf*ck...