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: remove deep keyword from EA.copy #27083

Merged
merged 5 commits into from Jun 27, 2019

Conversation

@jbrockmendel
Copy link
Member

commented Jun 27, 2019

Not clear on what type of deprecation cycle we want here

@jreback
Copy link
Contributor

left a comment

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

This comment has been minimized.

Copy link
@jreback

jreback Jun 27, 2019

Contributor

can you add a test in the pandas/tests/extension/constructors.py so it hits all EA's (for view & copy)

@jorisvandenbossche jorisvandenbossche changed the title remove deep keyword from EA.copy API: remove deep keyword from EA.copy Jun 27, 2019

@jorisvandenbossche

This comment has been minimized.

Copy link
Member

commented Jun 27, 2019

cc @TomAugspurger

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:

This comment has been minimized.

Copy link
@jreback

This comment has been minimized.

Copy link
@jreback

jreback Jun 27, 2019

Contributor

actually let's remove this for now @jbrockmendel

@jreback
Copy link
Contributor

left a comment

@jbrockmendel can you add a whatsnew note that remove the deep kwarg for EA

@jreback jreback added this to the 0.25.0 milestone Jun 27, 2019

@jorisvandenbossche

This comment has been minimized.

Copy link
Member

commented Jun 27, 2019

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.

@TomAugspurger

This comment has been minimized.

Copy link
Contributor

commented Jun 27, 2019

values = self.sp_values

def copy(self):
values = self.sp_values.copy()
return self._simple_new(values, self.sp_index, self.dtype)

This comment has been minimized.

Copy link
@jbrockmendel

jbrockmendel Jun 27, 2019

Author Member

@TomAugspurger do we need to copy the index here?

This comment has been minimized.

Copy link
@TomAugspurger

TomAugspurger Jun 27, 2019

Contributor

Not really sure. I don't know if they're designed for sharing across instances, or whether we mutate them inplace.

This comment has been minimized.

Copy link
@jorisvandenbossche

jorisvandenbossche Jun 27, 2019

Member

In any case in sparse.py, we don't mutate them inplace. I would also assume it is fine not to copy them.

@jreback

This comment has been minimized.

Copy link
Contributor

commented Jun 27, 2019

lgtm. ping on green.

@jorisvandenbossche jorisvandenbossche merged commit b387da8 into pandas-dev:master Jun 27, 2019

10 of 12 checks passed

pandas-dev.pandas Build #20190627.77 failed
Details
pandas-dev.pandas (Docs) Docs failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
pandas-dev.pandas (Checks) Checks succeeded
Details
pandas-dev.pandas (Linux py35_compat) Linux py35_compat succeeded
Details
pandas-dev.pandas (Linux py36_locale_slow) Linux py36_locale_slow succeeded
Details
pandas-dev.pandas (Linux py36_locale_slow_old_np) Linux py36_locale_slow_old_np succeeded
Details
pandas-dev.pandas (Linux py37_locale) Linux py37_locale succeeded
Details
pandas-dev.pandas (Linux py37_np_dev) Linux py37_np_dev succeeded
Details
pandas-dev.pandas (Windows py36_np15) Windows py36_np15 succeeded
Details
pandas-dev.pandas (Windows py37_np141) Windows py37_np141 succeeded
Details
pandas-dev.pandas (macOS py35_macos) macOS py35_macos succeeded
Details
@jorisvandenbossche

This comment has been minimized.

Copy link
Member

commented Jun 27, 2019

Thanks!

@jorisvandenbossche jorisvandenbossche referenced this pull request Jun 27, 2019

Closed

BUG: Categorical.copy deep kwarg #27024

2 of 4 tasks complete

@jbrockmendel jbrockmendel deleted the jbrockmendel:dcopy branch Jun 27, 2019

alimcmaster1 added a commit to alimcmaster1/pandas that referenced this pull request Jun 30, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.