-
-
Notifications
You must be signed in to change notification settings - Fork 140
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 Series.__new__ #722
Fix Series.__new__ #722
Conversation
@@ -208,55 +209,52 @@ class Series(IndexOpsMixin, NDFrame, Generic[S1]): | |||
_ListLike: TypeAlias = ArrayLike | dict[_str, np.ndarray] | list | tuple | Index | |||
__hash__: ClassVar[None] | |||
|
|||
# TODO: can __new__ be converted to __init__? Pandas implements __init__ |
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.
Can probably be done if #718 is possible
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.
For astype
, we created valid argument types for various types. It may not be fully inclusive, but we can do some restrictions in various places.
pandas-stubs/core/series.pyi
Outdated
index: Axes | None = ..., | ||
*, | ||
dtype: Literal["datetime64[ns]"], |
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.
Probably should be TimestampDtypeArg
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 all the other overloads would also need that: one overload with a specific data
and one for _ListLike
but a specific dtype
?
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.
My concern there would be that it would be too wide. In other words, there would be contents of lists that wouldn't work with certain dtypes. I think the one is in there for TimestampSeries
because it handles any kind of list.
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.
Probably should be
TimestampDtypeArg
I think this is the only change left to make.
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.
changed
pandas-stubs/core/series.pyi
Outdated
) -> TimestampSeries: ... | ||
@overload | ||
def __new__( | ||
cls, | ||
data: PeriodIndex, | ||
index: Axes | None = ..., | ||
dtype=..., | ||
dtype: Dtype = ..., |
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.
Seems like we need to create a PeriodDtypeArg
in _typing.pyi
with the valid values here
data: IntervalIndex[Interval[Timedelta]] | ||
| Interval[Timedelta] | ||
| Sequence[Interval[Timedelta]], | ||
data: IntervalIndex[Interval[_OrderableT]] |
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.
Needed to switch to bound
to simplify these overloads. All the other changes are related to this change.
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.
All looks good. Can you merge to main
to resolve conflicts?
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.
thanks @twoertwein
Series.__new__()
to be consistent in argument ordering. #717 (Replace xxxx with the Github issue number)assert_type()
to assert the type of any return valueI only added
*
for one overload as the implementation does not requires keyword-only arguments.I also removed
fastpath
as it is not documented.