Skip to content

Conversation

cmp0xff
Copy link
Contributor

@cmp0xff cmp0xff commented Oct 2, 2025

@cmp0xff cmp0xff force-pushed the feature/cmp0xff/624-extension-array-accumulate branch from 79a77ac to 309fdf4 Compare October 2, 2025 09:08
@cmp0xff cmp0xff changed the title feat(array): #624 accumulate feat(array): GH624 accumulate Oct 2, 2025
@cmp0xff cmp0xff changed the title feat(array): GH624 accumulate feat(array): #624 accumulate Oct 2, 2025
Copy link
Member

@loicdiridollou loicdiridollou left a comment

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,
Copy link
Collaborator

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:

  1. It encourages the use of Series[Int64Dtype] and I just don't think we would introduce that. Also, then Int64Dtype should really be in S1
  2. We can have a test with dtype="Int64", but then the results will just be an ExtensionArray for now, and if someone wants the real type, they can cast the result.

Copy link
Contributor Author

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: ...
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 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Dr-Irv
Copy link
Collaborator

Dr-Irv commented Oct 3, 2025

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.

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 @cmp0xff

Copy link
Contributor Author

@cmp0xff cmp0xff left a comment

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,
Copy link
Contributor Author

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: ...
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Dr-Irv Dr-Irv merged commit 7039781 into pandas-dev:main Oct 3, 2025
13 checks passed
@cmp0xff cmp0xff deleted the feature/cmp0xff/624-extension-array-accumulate branch October 3, 2025 15:10
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.

Version 2.0 Compatibility Tracker
3 participants