-
-
Notifications
You must be signed in to change notification settings - Fork 147
GH1327 Add overloead for date_range and timedelta_range #1333
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
Changes from 4 commits
2a99f70
f26b710
584e288
9bf345a
56be8d4
fc35578
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -79,13 +79,53 @@ class TimedeltaIndex( | |
def to_series(self, index=..., name: Hashable = ...) -> TimedeltaSeries: ... | ||
def shift(self, periods: int = 1, freq=...) -> Self: ... | ||
|
||
@overload | ||
def timedelta_range( | ||
start: TimedeltaConvertibleTypes | None = None, | ||
end: TimedeltaConvertibleTypes | None = None, | ||
periods: int | None = None, | ||
start: TimedeltaConvertibleTypes = ..., | ||
end: TimedeltaConvertibleTypes = ..., | ||
periods: int = ..., | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same issue here as with |
||
*, | ||
name: Hashable | None = None, | ||
closed: Literal["left", "right"] | None = None, | ||
unit: None | str = ..., | ||
) -> TimedeltaIndex: ... | ||
@overload | ||
def timedelta_range( | ||
start: TimedeltaConvertibleTypes = ..., | ||
periods: int = ..., | ||
freq: None = None, | ||
name: Hashable | None = None, | ||
closed: Literal["left", "right"] | None = None, | ||
*, | ||
unit: None | str = ..., | ||
) -> TimedeltaIndex: ... | ||
@overload | ||
def timedelta_range( | ||
start: TimedeltaConvertibleTypes = ..., | ||
*, | ||
periods: int = ..., | ||
freq: str | DateOffset | Timedelta | dt.timedelta | None = None, | ||
name: Hashable | None = None, | ||
closed: Literal["left", "right"] | None = None, | ||
unit: None | str = ..., | ||
) -> TimedeltaIndex: ... | ||
@overload | ||
def timedelta_range( | ||
start: TimedeltaConvertibleTypes = ..., | ||
end: TimedeltaConvertibleTypes = ..., | ||
*, | ||
freq: str | DateOffset | Timedelta | dt.timedelta | None = None, | ||
name: Hashable | None = None, | ||
closed: Literal["left", "right"] | None = None, | ||
unit: None | str = ..., | ||
) -> TimedeltaIndex: ... | ||
@overload | ||
def timedelta_range( | ||
*, | ||
end: TimedeltaConvertibleTypes, | ||
periods: int, | ||
freq: str | DateOffset | Timedelta | dt.timedelta | None = None, | ||
name: Hashable | None = None, | ||
closed: Literal["left", "right"] | None = None, | ||
unit: None | str = ..., | ||
) -> TimedeltaIndex: ... |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1563,6 +1563,13 @@ def test_timedelta_range() -> None: | |
pd.TimedeltaIndex, | ||
) | ||
|
||
if TYPE_CHECKING_INVALID_USAGE: | ||
day1 = pd.Timedelta(1, unit="D") | ||
day10 = pd.Timedelta(10, unit="D") | ||
pd.timedelta_range( | ||
day1, day10, 10, "D" # type: ignore[call-overload] # pyright: ignore[reportArgumentType] | ||
) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should add checks where only 0, 1 or 2 of the 3 arguments are specified, as that should fail as well. |
||
|
||
|
||
def test_dateoffset_freqstr() -> None: | ||
offset = DateOffset(minutes=10) | ||
|
@@ -1743,6 +1750,13 @@ def test_creating_date_range() -> None: | |
dr = pd.date_range(start="2021-12-01", periods=24, freq="h") | ||
check(assert_type(dr.strftime("%H:%M:%S"), pd.Index), pd.Index, str) | ||
|
||
if TYPE_CHECKING_INVALID_USAGE: | ||
day1 = pd.Timestamp("2023-04-03") | ||
day10 = pd.Timedelta("2023-04-08") | ||
pd.date_range( | ||
day1, day10, 10, "D" # type: ignore[call-overload] # pyright: ignore[reportCallIssue] | ||
) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same comment - need to check that 0, 1 or 2 arguments specified causes a failure. |
||
|
||
|
||
def test_timestamp_to_list_add() -> None: | ||
# https://github.com/microsoft/python-type-stubs/issues/110 | ||
|
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 would allow
pd.date_range()
to be accepted.The error message is " Of the four parameters: start, end, periods, and freq, exactly three must be specified".
So if you want this to work, you have to have overloads where 3 are required. You can't have
= ...
on any of them (or default values either), because that means the argument is optional.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.
The point where I am a bit unsure is that most of the usage I have seen it just about passing start and end, freq is often left alone so the usage does not reflect the docs.
Should we force the user to use three out of the four or allow only two?
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.
So I think the docs and error message are a bit misleading.
You'll need to do a test to determine when 2 are allowed. I think the rule is as follows:
freq
is specified, you need at least 2 out of the 3 ofstart
,end
,periods
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.
If I look at the docs, they don't even use three exactly, when looking at the code they are using the fact that freq defaults to None but is then replaced by D for day.
https://pandas.pydata.org/docs/dev/reference/api/pandas.timedelta_range.html#pandas.timedelta_range
See https://github.com/pandas-dev/pandas/blob/78ec27668163808b3488da6ed2c64d0b8e9bfbc8/pandas/core/indexes/timedeltas.py#L330-L331 for the code where they change freq=None to freq="D"
Should we force the user to do 3 args even if the docs example only uses 2.
Happy to open a PR in the pandas repo to ask for clarification.
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.
Sorry took a bit of time to find all the docs and you had answered before.
Let me setup a quick test to validate that we need two out of three of
start
,end
, andperiods
.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 the freq is indeed the argument that does not need to be passed (it is also the last one in the order).
I think the correct way to type hint is like you said forcing 2 out of the 3 with freq being an
str | None = ...
without the need to be passed when you pass two other. I have also tried passing four and that works as long as one of the three is None (yet it would be a strange use case of passing None since it is a default).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.
pandas-dev/pandas#62161
I have asked for clarifications.
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 have read and reread the docs, it is a bit confusing at first glance but I will add all the tests for it.