feat(array): #624 accumulate by cmp0xff · Pull Request #1400 · pandas-dev/pandas-stubs (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation8 Commits4 Checks13 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
- Closes Version 2.0 Compatibility Tracker #624
Added cumsum, cumprod, cummin and cummax to the ExtensionArray interface via _accumulate(partially resolving it) - Relates to Nullable dtypes for Series.__new__ and astype #1395
- Tests added
feat(array): #624 accumulate feat(array): GH624 accumulate
feat(array): GH624 accumulate feat(array): #624 accumulate
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, thanks @cmp0xff ! @Dr-Irv let me know if you see something missing but checked the docs and it is aligned, should we put Literal["cummin", "cummax", "cumsum", "cumprod"] into it's own type, it is only used there so in favor of not doing so but please advise.
| assert_type( |
|---|
| pd.Series([1], dtype="category").array, |
| pd.Categorical, |
| cast("Series[Int64Dtype]", Series([1, NA], dtype="Int64")).array, |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should not have a test like this for 2 reasons:
- It encourages the use of
Series[Int64Dtype]and I just don't think we would introduce that. Also, thenInt64Dtypeshould really be inS1 - We can have a test with
dtype="Int64", but then the results will just be anExtensionArrayfor now, and if someone wants the real type, they can cast the result.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment on lines 471 to 473
| def __get__( |
|---|
| self, instance: IndexOpsMixin[Int64Dtype], owner: type[IndexOpsMixin] |
| ) -> IntegerArray: ... |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this should exist because Int64Dtype is in S1, and I don't want to introduce that now, because it will open up a whole set of new issues.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, thanks @cmp0xff ! @Dr-Irv let me know if you see something missing but checked the docs and it is aligned, should we put
Literal["cummin", "cummax", "cumsum", "cumprod"]into it's own type, it is only used there so in favor of not doing so but please advise.
I don't think we need to create a new type for that. The extension array stuff isn't widely used.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Series[Int64Dtype] removed
| assert_type( |
|---|
| pd.Series([1], dtype="category").array, |
| pd.Categorical, |
| cast("Series[Int64Dtype]", Series([1, NA], dtype="Int64")).array, |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment on lines 471 to 473
| def __get__( |
|---|
| self, instance: IndexOpsMixin[Int64Dtype], owner: type[IndexOpsMixin] |
| ) -> IntegerArray: ... |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.