ENH: Add lazy copy to astype
State: closed

xref https://github.com/pandas-dev/pandas/issues/49473

- [ ] closes #xxxx (Replace xxxx with the GitHub issue number)
- [ ] [Tests added and passed](https://pandas.pydata.org/pandas-docs/dev/development/contributing_codebase.html#writing-tests) if fixing a bug or adding a new feature
- [ ] All [code checks passed](https://pandas.pydata.org/pandas-docs/dev/development/contributing_codebase.html#pre-commit).
- [ ] Added [type annotations](https://pandas.pydata.org/pandas-docs/dev/development/contributing_codebase.html#type-hints) to new arguments/methods/functions.
- [ ] Added an entry in the latest `doc/source/whatsnew/vX.X.X.rst` file if fixing a bug or adding a new feature.


Labels:
name: Copy / view semantics

Comments:
cc @jorisvandenbossche 

I think this is ready for another look
> For the case of "object -> some extension dtype" casting, we should probably always do `copy=True`, because I don't think we can rely on `_from_sequence` to do the correct thing (although for this specific case I also don't think that `StringArray._from_sequence(arr, copy=False)` should mutate `arr` in place. I would expect to _only_ not make a copy if it's not needed, i.e. if it can just view the original data and no modification needs to be done)

For generic extension dtypes, we can test with the test decimal extension dtype (although first have to fix its constructor to not ignore the `copy` keyword). This is also a case of storing an object-dtype array under the hood, so object->decimal _can_ be a view if we pass `copy=False` down.

But it might also not only be an issue for object dtype. For any numpy dtype -> extension dtype, this eventually goes through `EA._from_sequence(..)`. So we should maybe pass `copy=True` for all those cases (for generic EAs, we can special case for our own internal ones)


 
Added a hacky decimal test. What's the most efficient way of figuring out our own eas?
So the reason that the decimal test you added works without actual code change is because the ExtensionDtype.kind attribute for that dtype is also object (that's the default on the base class ExtensionDtype). And then you get the `elif is_object_dtype(dtype) and new_dtype.kind == "O":` returning True, so we keep track of references. But if the `kind` is not object, we might end up in the final `return False`, and we might be incorrectly assuming the result is a copy and not keep track of references.

And I think that all our own extension dtypes (that are not the nullable ones) have a `kind` of "O" (categorical, period, interval, ..), and so those don't run into the issue. 
We have one test extension dtype that doesn't have to be of `kind` object, and that is the `FloatAttrDtype`. If I update that dtype to have the correct `kind` of float:

```diff
diff --git a/pandas/tests/extension/array_with_attr/array.py b/pandas/tests/extension/array_with_attr/array.py
index d9327ca9f2..94923b1b32 100644
--- a/pandas/tests/extension/array_with_attr/array.py
+++ b/pandas/tests/extension/array_with_attr/array.py
@@ -20,6 +20,7 @@ class FloatAttrDtype(ExtensionDtype):
     type = float
     name = "float_attr"
     na_value = np.nan
+    kind = "f"
 
     @classmethod
     def construct_array_type(cls) -> type_t[FloatAttrArray]:
```

then I observe the following bug with the current branch:
```
In [1]: from pandas.tests.extension.array_with_attr import FloatAttrDtype

In [2]: s = pd.Series([1, 2, 3], dtype="float")

In [3]: s2 = s.astype(FloatAttrDtype())

In [4]: s[0] = 10.0   # <- this is fine, s2 not changed (since a copy was made)

In [5]: s2
Out[5]: 
0    1.0
1    2.0
2    3.0
dtype: float_attr

In [6]: pd.options.mode.copy_on_write = True

In [7]: s = pd.Series([1, 2, 3], dtype="float")

In [8]: s2 = s.astype(FloatAttrDtype())

In [9]: s[0] = 10.0   # but with CoW enabled, this does mutate s2

In [10]: s2
Out[10]: 
0    10.0
1     2.0
2     3.0
dtype: float_attr
```

I don't know if there are many EAs out there that would run into this, though .. But when we end up calling `EA._from_sequence` (calling astype(EAdtype) with a dtype we don't implement ourselves), I think we should either assume that the result _can_ be view (and so track refs), or either in this case do pass `copy=True` to `_from_sequence`, instead of `copy=False` now when CoW is enabled.

(I am maybe also going a bit too far with my review here, it doesn't necessarily have to be perfect for all corner cases in the first PR :))
Yeah I agree with third party eas. I actually like your suggestion of tracking refs more than passing copy=True. I don't know right now how to determine if we have a 3rd party ea, that's why I did not implement it yet.
> I don't know right now how to determine if we have a 3rd party ea, that's why I did not implement it yet. 

I think all our internal EAs will be captured by one of the specific checks in `astype_is_view`, so whenever you end up in the end but the target dtype is an ExtensionDtype?

The drawback of not passing copy=True but keeping a ref, is that we might be doing an unnecessary copy later one. Of course with the benefit we might be avoiding a copy during the astype. No idea which one of the two would be most common, though. So given that, either solution is probably fine for now.
I adjusted the view check a bit to return True more often. This should avoid missing no-copy ops. Can also iterate later if we want to do an eager copy In astype and set the references to False. But considering 

```
df = df.astype()
```

deferring the copy makes the most sense to me

In general: let's get this merged with the current version of `astype_is_view`, and fine tune that in follow-up PRs?
Yeah I think we cover most general cases now. I'd suggest tracking further optimisations in an issue.

Edit: I think we don't miss any potential views now while we already cover most cases.
@jorisvandenbossche 

Are you ok with merging this when greenish? One test was failing for array manager, is fixed now. 

This opens up a few other things.
Yes, certainly, that will also make my constructor PR easier to update.