-
-
Notifications
You must be signed in to change notification settings - Fork 118
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
fix matplotlib imports, pd.util #776
Conversation
@@ -239,7 +239,7 @@ class PlotAccessor: | |||
**kwargs: Any, | |||
) -> pd.Series: ... | |||
@overload | |||
def line( | |||
def line( # type: ignore[misc] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for me to understand why these changes are needed: mypy&pyright cannot resolve Axis, so it is treated as Any/Unknown, therefore the overloads overlap. Is that correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is because matplotlib
changed the way that Axes
is declared.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked at their code: maybe the from ... import *
or the many class decorators throw mypy/pyright off.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this has to do with matplotlib
, but is rather a change in mypy
(Saw similar things in xarray
)
Here, I think the problem may actually be that you have a default value indicated for both the True
and False
case of the subplots kwarg, when I think that should only apply in one case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ksunden I think you are right - one of the overloads should require a user-provided input!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -13,6 +13,7 @@ | |||
import pandas as pd | |||
from pandas import Grouper | |||
from pandas.api.extensions import ExtensionArray | |||
import pandas.util as pdutil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a # TODO: github.com/pandas-dev/pandas/issues/55023
I assume this would fix the workaround?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so. In next commit.
@@ -1,6 +1,8 @@ | |||
import io | |||
from typing import Any | |||
|
|||
from matplotlib.axes import Axes | |||
from matplotlib.figure import Figure |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this just hides the explicit re-export issue (I'm surprised that mypy/pyright are fooled that easily).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I think matplotlib
got updated, and now plt.Axes
and plt.Figure
aren't valid any more in code from a type checking perspective.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Except for adding a comment about pd.util
it looks good to me.
(Should probably open an issue/pr at matplotlib so that Axis/Figure are ex-exported)
Thanks @Dr-Irv ! |
Create the matplotlib issue at matplotlib/matplotlib#26812 |
CI was failing due to matplotlib changes. This fixes those issues.