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

Series(Mapping) #843

Merged
merged 6 commits into from
Dec 26, 2023
Merged

Series(Mapping) #843

merged 6 commits into from
Dec 26, 2023

Conversation

twoertwein
Copy link
Member

@@ -294,7 +297,7 @@ class Series(IndexOpsMixin[S1], NDFrame):
@overload
def __new__(
cls,
data: S1 | _ListLike[S1] | dict[int, S1] | dict[_str, S1],
data: S1 | Mapping[Any, S1] | _ListLike[S1],
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not happy with _ListLike but a Mapping is also Iterable.

Copy link
Collaborator

@Dr-Irv Dr-Irv left a comment

Choose a reason for hiding this comment

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

I think we should consider using Mapping[Hashable, _appropriate_type_here] instead of Mapping[Any, _approprirate_type_here], since we know that the keys of the dict will end up as column names.

pandas-stubs/core/series.pyi Outdated Show resolved Hide resolved
pandas-stubs/core/series.pyi Outdated Show resolved Hide resolved
@@ -284,7 +287,7 @@ class Series(IndexOpsMixin[S1], NDFrame):
@overload
def __new__(
cls,
data: Scalar | _ListLike | dict[int, Any] | dict[_str, Any] | None,
data: Scalar | Iterable | None,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think Iterable is correct here, because we don't (or shouldn't) accept a set.

Copy link
Member Author

Choose a reason for hiding this comment

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

Would it be correct to have one Never-overload for set and then simply use data: Any + specified dtype?

Copy link
Collaborator

@Dr-Irv Dr-Irv Dec 26, 2023

Choose a reason for hiding this comment

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

I don't know. I'd rather keep things narrow by just using Scalar | _ListLike | dict[Hashable, Any] | None . Or maybe we can use Mapping[Hashable, Scalar] instead of dict[Hashable, Any] .

My feeling is that we should make small increments when we change the stubs so as to not widen things too much.

Copy link
Member Author

Choose a reason for hiding this comment

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

My feeling is that we should make small increments when we change the stubs so as to not widen things too much.

[sorry, I edited your comment by accident - wanted to quote reply]

I agree about that! But I also feel like we are making ourselves too much work by trying to be as restrictive as possible.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree about that! But I also feel like we are making ourselves too much work by trying to be as restrictive as possible.

Possibly. But I'd rather let the community report issues, and we really haven't had too many issues where we made things too restrictive.

See what I wrote here 18 months ago about this topic: https://github.com/pandas-dev/pandas-stubs/blob/main/docs/philosophy.md#narrow-vs-wide-arguments

@@ -304,7 +307,7 @@ class Series(IndexOpsMixin[S1], NDFrame):
@overload
def __new__(
cls,
data: Scalar | _ListLike | dict[int, Any] | dict[_str, Any] | None = ...,
data: Scalar | Iterable | None = ...,
Copy link
Collaborator

Choose a reason for hiding this comment

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

same comment as above about Iterable

Copy link
Collaborator

@Dr-Irv Dr-Irv left a comment

Choose a reason for hiding this comment

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

Thanks @twoertwein

@Dr-Irv Dr-Irv merged commit 72b22b5 into pandas-dev:main Dec 26, 2023
13 checks passed
@twoertwein twoertwein deleted the series_mapping branch February 10, 2024 20:30
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.

dict argument to pd.Series constructor should allow Timestamp and tuple as keys
2 participants