API: remove deep keyword from EA.copy by jbrockmendel · Pull Request #27083 · pandas-dev/pandas (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
Conversation15 Commits5 Checks0 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 }})
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would be ok with just making this change (no deprecation cycle), @jorisvandenbossche
| @@ -101,10 +101,8 @@ def take(self, indexer, allow_fill=False, fill_value=None): |
|---|
| allow_fill=allow_fill) |
| return self._from_sequence(result) |
| def copy(self, deep=False): |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add a test in the pandas/tests/extension/constructors.py so it hits all EA's (for view & copy)
remove deep keyword from EA.copy API: remove deep keyword from EA.copy
Looks good to me. As usual (sorry ...), I would only add view() if we actually need it (meaning: use it internally)
| """ |
|---|
| raise AbstractMethodError(self) |
| def view(self, dtype=None) -> ABCExtensionArray: |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jbrockmendel can you add a whatsnew note that remove the deep kwarg for EA
Slight preference for deprecation, but don't have strong thoughts here.
So a user only runs into this when they directly interface with an ExtensionArray themselves (as the uses within pandas get fixed). Further, for EA authors only implementing the EA itself, it's not a backwards incompatible change, as we don't have a problem that the EA still has the keyword, it will just never be called from inside pandas.
It seems that users doing Series.array.copy(deep=True) is something very specific, and also only introduced very recently (eg Categorical is longest out there, but has actually no copy keyword). IntegerArray and IntervalArray eg does have one.
Sounds good.
On Thu, Jun 27, 2019 at 3:04 PM Joris Van den Bossche < ***@***.***> wrote: Slight preference for deprecation, but don't have strong thoughts here. So a user only runs into this when they directly interface with an ExtensionArray themselves (as the uses within pandas get fixed). Further, for EA authors only implementing the EA itself, it's not a backwards incompatible change, as we don't have a problem that the EA still has the keyword, it will just never be called from inside pandas. It seems that users doing Series.array.copy(deep=True) is something very specific, and also only introduced very recently (eg Categorical is longest out there, but has actually no copy keyword). IntegerArray and IntervalArray eg does have one. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#27083?email_source=notifications&email_token=AAKAOIXCWW7BQBAYLMGNM3TP4UMMZA5CNFSM4H37CG42YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODYYG4YY#issuecomment-506490467>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAKAOIVQTC74QUOSYMYYFRTP4UMMZANCNFSM4H37CG4Q> .
| values = self.sp_values |
|---|
| def copy(self): |
| values = self.sp_values.copy() |
| return self._simple_new(values, self.sp_index, self.dtype) |
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.
Not really sure. I don't know if they're designed for sharing across instances, or whether we mutate them inplace.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In any case in sparse.py, we don't mutate them inplace. I would also assume it is fine not to copy them.
