Skip to content

TYP: overload maybe_downcast_numeric and maybe_downcast_to_dtype #46929

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

Merged
merged 2 commits into from
May 6, 2022

Conversation

iasoon
Copy link
Contributor

@iasoon iasoon commented May 2, 2022

xref #37715

Overload type definitions for these methods so that passing in an ndarray returns an ndarray.
This allows removing two more ignored mypy errors.

Copy link
Member

@rhshadrach rhshadrach left a comment

Choose a reason for hiding this comment

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

Instead of using overloads, does it make sense to have an ArrayLikeT, i.e. a TypeVar bound to np.ndarray | ExtensionArray?

cc @simonjayhawkins @Dr-Irv

@Dr-Irv
Copy link
Contributor

Dr-Irv commented May 2, 2022

Instead of using overloads, does it make sense to have an ArrayLikeT, i.e. a TypeVar bound to np.ndarray | ExtensionArray?

cc @simonjayhawkins @Dr-Irv

I think (but I'm not sure) that the times to use a TypeVar are when subclassing gets involved with respect to self . I think the overload approach is correct here, but also agree with @twoertwein that with ArrayLike including np.ndarray, it's better to have one overload for np.ndarray and a different one for ExtensionArray

@iasoon iasoon force-pushed the typing/maybe_downcast branch from 443b166 to d865197 Compare May 3, 2022 16:40
@iasoon
Copy link
Contributor Author

iasoon commented May 3, 2022

Thanks for the comments!
I played around a bit with the typevar route, but I do not think it is sound in this case: both methods will return np.ndarray when passed np.ndarray, but could return either ExtensionArray or np.ndarray when passed ExtensionArray.
I agree non-overlapping overloads are cleaner here, I resolved that.

@twoertwein twoertwein merged commit d42a148 into pandas-dev:main May 6, 2022
@twoertwein
Copy link
Member

Thank you @iasoon!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants