Non-Deep copy for Categorical?

[jbrockmendel]

Categorical.copy passes values=self._codes.copy(), and doesn't have a deep=False kwarg (which the base ExtensionArray does). This is causing trouble in #26954. Is this intentional @TomAugspurger?

Pandas-internal EAs with copy behavior not respecting deep kwarg:

• Categorical (no kwarg at all)
• DatetimeLike (ignores kwarg)
• PandasArray

IntegerArray, IntervalArray, and SparseArray look correct.

[TomAugspurger]

Probably a bug.

[jorisvandenbossche]

In general, we never really properly defined the expected semantics for EA.copy (or exposed it through eg Series.copy) -> #22314

Also currently, the default of deep is False, but I assume this should actually be True (if you use the same meaning as for Series). Or, we could just remove this keyword altogether for EAs (eg ndarray also has no deep argument)

[TomAugspurger]

Agreed that it should be deep by default.

…

On Mon, Jun 24, 2019 at 11:17 AM Joris Van den Bossche < ***@***.***> wrote: In general, we never really properly defined the expected semantics for EA.copy (or exposed it through eg Series.copy) -> #22314 <#22314> Also currently, the default of deep is False, but I assume this should actually be True (if you use the same meaning as for Series). Or, we could just remove this keyword altogether for EAs (eg ndarray also has no deep argument) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#26995?email_source=notifications&email_token=AAKAOITOPIJTIACYIP4FVXLP4DXQ3A5CNFSM4H2VY662YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODYNOKUI#issuecomment-505079121>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAKAOIQ7S4JEWYHBUF2NUBTP4DXQ3ANCNFSM4H2VY66Q> .

[jorisvandenbossche]

Is there a use case for a non-deep one?

[jreback]

actually need to be careful with this

we don’t normally deep copy the cells of object array
eg day you have dicts or lists inside a Series cell

as that is not python default behavior

however it has causes some bugs over time

[jorisvandenbossche]

@jbrockmendel we don't get notified of edits of the first post, but I think your updated information is relevant for the discussion: basically none of the EAs implement this.

So before actually starting to implement this, to repeat my previous questions:

• shouldn't deep=True be the default?
• is there actually a use case for deep=False, or can we just remove this keyword alltogether?

[jbrockmendel]

shouldn't deep=True be the default?

I would be +1 on this.

is there actually a use case for deep=False, or can we just remove this keyword alltogether?

The only thing that comes to mind is that this is kind of like a view that could be used for some copy-on-write behavior, but if that's the idea then we should just use view. I'd be on board with removing the kwarg.

[jbrockmendel]

is there actually a use case for deep=False, or can we just remove this keyword alltogether?

Come to think of it, we don't have a non-copying view method, which would be helpful for reshape.

[jorisvandenbossche]

@jbrockmendel can you explain a bit more how that would work? (why is a non-copy view needed there?)