Skip to content
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

API: ExtensionArray._concat_same_type doesn't define axis #47514

Open
mroeschke opened this issue Jun 26, 2022 · 5 comments
Open

API: ExtensionArray._concat_same_type doesn't define axis #47514

mroeschke opened this issue Jun 26, 2022 · 5 comments
Labels
API Design ExtensionArray Extending pandas with custom dtypes or arrays. Ice Cream Agreement Issues that would be addressed by the Ice Cream Agreement from the Aug 2023 sprint Reshaping Concat, Merge/Join, Stack/Unstack, Explode

Comments

@mroeschke
Copy link
Member

Mentioned in #46008

The signature for ExtensionArray._concat_same_type is defined as def _concat_same_type(cls, sequence).

Some internal extension arrays define the method with an axis argument e.g. BaseMaskedArray._concat_same_type(cls, sequence, axis)

Some internal routines expect _concat_same_type to accept axis e.g _concat_datetime (hit by checking kind on the dtype).

I imagine either:

  1. Internally, remove axis for internally defined _concat_same_type(cls, sequence, axis) (imagine not feasible)
  2. Add axis to the ExtensionArray._concat_same_type signature.
@mroeschke mroeschke added Reshaping Concat, Merge/Join, Stack/Unstack, Explode API Design ExtensionArray Extending pandas with custom dtypes or arrays. labels Jun 26, 2022
@jbrockmendel
Copy link
Member

Some internal routines expect _concat_same_type to accept axis e.g _concat_datetime (hit by checking kind on the dtype).

i expect this is a problem for pyarrow[datetime64]? i think the Technically Correct check would be dtype.kind in ["m", "M"] and not is_1d_only_ea_dtype(dtype)

@mroeschke
Copy link
Member Author

i expect this is a problem for pyarrow[datetime64]?

Yeah I think testing with that dtype first uncovered this, but I imagine any custom EA who's EADtype is kind m/M would be impacted

@jbrockmendel
Copy link
Member

Yeah I think testing with that dtype first uncovered this, but I imagine any custom EA who's EADtype is kind m/M would be impacted

This should now be fixed?

@mroeschke
Copy link
Member Author

I think the original issue is fixed but shouldn't subclasses have the same _concat_same_type signature as the base method?

@jbrockmendel
Copy link
Member

I think the original issue is fixed but shouldn't subclasses have the same _concat_same_type signature as the base method?

I think this is going to be an issue as long as some-but-not-all EAs support 2D. I guess we could add a mostly-irrelevant axis keyword to the base class but that would confuse 3rd party authors.

@jbrockmendel jbrockmendel added the Ice Cream Agreement Issues that would be addressed by the Ice Cream Agreement from the Aug 2023 sprint label Dec 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Design ExtensionArray Extending pandas with custom dtypes or arrays. Ice Cream Agreement Issues that would be addressed by the Ice Cream Agreement from the Aug 2023 sprint Reshaping Concat, Merge/Join, Stack/Unstack, Explode
Projects
None yet
Development

No branches or pull requests

2 participants