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.

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

TYP: mostly io, plotting #37059

Merged
merged 7 commits into from
Oct 14, 2020
Merged

TYP: mostly io, plotting #37059

merged 7 commits into from
Oct 14, 2020

Conversation

jbrockmendel
Copy link
Member

  • closes #xxxx
  • tests added / passed
  • passes black pandas
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff
  • whatsnew entry

cc @simonjayhawkins can i get your thoughts on two remaining mypy complaints

pandas/io/excel/_base.py:795: error: "Callable[[ExcelWriter], Tuple[str, ...]]" has no attribute "__iter__" (not iterable)  [attr-defined]
pandas/io/stata.py:892: error: unused 'type: ignore' comment

The excel one is because it doesn't recognize

@property
@abstractmethod
def supported_extensions(self) -> Tuple[str, ...]:

as returning Tuple[str, ...]. I expect if that were resolved it would still complain bc subclasses just pin the tuple as a class attribute instead of a property (easy to make a property, but much more verbose which im not wild about)

The stata unused type:ignore becomes used once we remove stata from the setup.cfg exclusion. The only working way I've found to make mypy accept TYPE_MAP: List[Union[int, str]] = list(range(251)) + list("abcd") is with

TYPE_MAP: List[Union[int, str]] = cast(List[Union[int, str]], list(range(251))) + list("abcd")

which i find much less clear than the # type:ignored version.

@jreback jreback added Typing type annotations, mypy/pyright type checking Visualization plotting labels Oct 11, 2020
@jbrockmendel
Copy link
Member Author

punted on issues in the OP, so this should be ready

@simonjayhawkins
Copy link
Member

cc @simonjayhawkins can i get your thoughts on two remaining mypy complaints

pandas/io/excel/_base.py:795: error: "Callable[[ExcelWriter], Tuple[str, ...]]" has no attribute "__iter__" (not iterable)  [attr-defined]

see #28339 (comment)

@jbrockmendel
Copy link
Member Author

Makes sense, thanks. Are you OK with the solution here?

@simonjayhawkins
Copy link
Member

Makes sense, thanks. Are you OK with the solution here?

from the previous discussion, #28339 (comment)

Sure you could just remove the abstract requirement altogether for supported_extensions and make it a class variable, but that would lose the abstract nature of it.

maybe type: ignore for now and reference the mypy issue, python/mypy#7760

The crux of the issue seems to be python/mypy#7760 (comment)

I guess my real issue then is Python language's lack of notation for abstract class attributes.

The code does work. so we shouldn't really need to change it.

if not is_list_like(axes):
return np.array([axes])
elif isinstance(axes, (np.ndarray, ABCIndexClass)):
return axes.ravel()
return np.asarray(axes).ravel()
Copy link
Member

Choose a reason for hiding this comment

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

The issue here is that the types are not refined by ABCIndexClass?

for a numpy array, this impacts perf.

%timeit np.asarray(axes).ravel()
# 724 ns ± 11.8 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)

%timeit axes.ravel()
# 303 ns ± 9.35 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)

Is this important?

maybe split the elif into two or ignore till we resolve the ABC issue.

Copy link
Member

Choose a reason for hiding this comment

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

or can we cast to Union[np.ndarray, "Index"]

Copy link
Member Author

Choose a reason for hiding this comment

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

Index.ravel behavior was just deprecated, so the np.asarray retains the current behavior

Copy link
Member

@simonjayhawkins simonjayhawkins left a comment

Choose a reason for hiding this comment

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

Thanks @jbrockmendel lgtm

@jreback jreback added this to the 1.2 milestone Oct 14, 2020
@jreback jreback merged commit 3d29aee into pandas-dev:master Oct 14, 2020
@jbrockmendel jbrockmendel deleted the typ-4 branch October 14, 2020 14:41
JulianWgs pushed a commit to JulianWgs/pandas that referenced this pull request Oct 26, 2020
kesmit13 pushed a commit to kesmit13/pandas that referenced this pull request Nov 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Typing type annotations, mypy/pyright type checking Visualization plotting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants