GH1169 Improve parameter types for DataFrame.pct_change and Series.pct_change
#1194
Conversation
yangdanny97
changed the title
DataFrame.pct_change and Series.pct_changeDataFrame.pct_change and Series.pct_change
| @@ -1987,11 +1987,13 @@ class DataFrame(NDFrame, OpsMixin, _GetItemHack): | |||
| def ne(self, other, axis: Axis = ..., level: Level | None = ...) -> Self: ... | |||
| def pct_change( | |||
| self, | |||
| periods: int = ..., | |||
| periods: int = 1, | |||
There was a problem hiding this comment.
You can't do this in a stubs. Please leave it as periods: int = ... because the periods argument is optional.
There was a problem hiding this comment.
Sorry, could you please clarify? The argument is still optional when written like this.
I see a few other places where the default is written when it's a simple literal, like https://github.com/pandas-dev/pandas-stubs/blob/main/pandas-stubs/core/frame.pyi#L317
There was a problem hiding this comment.
In the types ... means you just take whatever the value is in the definition of the method (in the pandas code so that allows to keep track of the default value in only one place).
The itertools you are pointing to is an overload which is different than the function you are modifying here. The overload adapts the return type to a certain type of input types you are passing. For the example you provide, it is hard-coded since you are forcing the value to be None and that does not give the user for anything else, whereas you see that the methods also allows for the type str:
pandas-stubs/pandas-stubs/core/frame.pyi
Lines 312 to 314 in a6faaf3
There was a problem hiding this comment.
Understood, thank you
| @@ -1987,11 +1987,13 @@ class DataFrame(NDFrame, OpsMixin, _GetItemHack): | |||
| def ne(self, other, axis: Axis = ..., level: Level | None = ...) -> Self: ... | |||
| def pct_change( | |||
| self, | |||
| periods: int = ..., | |||
| periods: int = 1, | |||
| fill_method: None = ..., | |||
| limit: int | None = ..., | |||
There was a problem hiding this comment.
limit has been deprecated, so remove this
| fill_method: None = ..., | ||
| limit: int | None = ..., | ||
| freq=..., | ||
| **kwargs: Any, # TODO: make more precise https://github.com/pandas-dev/pandas-stubs/issues/1169 | ||
| freq: Frequency | dt.timedelta | None = None, |
There was a problem hiding this comment.
| freq: Frequency | dt.timedelta | None = None, | |
| freq: DateOffset| dt.timedelta | None = None, |
Frequency is too wide. Should be changed in shift too. That's old code.
| freq: Frequency | dt.timedelta | None = None, | ||
| *, | ||
| axis: AxisIndex = ..., | ||
| fill_value: object | None = ..., |
There was a problem hiding this comment.
| fill_value: object | None = ..., | |
| fill_value: Scalar | NAType | None = ..., |
object is too wide.
| periods: int = 1, | ||
| fill_method: None = ..., | ||
| limit: int | None = ..., | ||
| freq=..., | ||
| **kwargs: Any, # TODO: make more precise https://github.com/pandas-dev/pandas-stubs/issues/1169 | ||
| freq: Frequency | timedelta | None = None, | ||
| *, | ||
| axis: AxisIndex = ..., | ||
| fill_value: object | None = ..., |
There was a problem hiding this comment.
Same comment as above on DataFrame.pct_change() . Also, remove axis as it is not used in Series.pct_change().
This PR does the following for the two functions in the title:
replace kwargs with keyword-only parameters for
pct_changebased on parameters fromshiftreplace
...w/ simple defaults when applicable based on docs: https://pandas.pydata.org/docs/reference/api/pandas.Series.pct_change.htmlmake
fill_methodforSeries.pct_changebeNone, just likeDataFrame.pct_changesince all other values are deprecatedCloses type
kwargsinpct_changeaccording to params inshift#1169Tests added: Please use
assert_type()to assert the type of any return valuecc @MarcoGorelli should these have tests? they're not overloaded so I'm not sure if it's necessary