CoW: add readonly flag to ExtensionArrays, return read-only EA/ndarray in .array/EA.to_numpy()

[jorisvandenbossche]

Addresses one of the remaining TODO items from #48998

Similar as #51082 and some follow-up PRs, ensuring we also mark EAs as read-only like we do for numpy arrays, when the user gets the underlying EA from a pandas object.
For that purpose, added a _readonly attribute to the EA class that is False by default.

Still need to add more tests and fix a bunch of tests

closes #58007

[simonjayhawkins]

why not use arr.flags.writeable to be consistent with numpy?

[jorisvandenbossche]

Because this was easier for a quick POC ;)
It would indeed keep it more consistent in usage, so that might be a reason to add a flags attribute, so code that needs to work with both ndarray or EA can use one code path. But I don't think we would ever add any of the other flags that numpy has, so not sure it would then be worth to add a nested attribute for this.

[jbrockmendel]

should this be done below the setting of na_value on L616?

[jorisvandenbossche]

I don't think so, because in that case the result array is already a copy, so no need to take a read-only view in that case

[jbrockmendel]

i get why .values and .array are made read-only, but why are we bothering with to_numpy?

[jorisvandenbossche]

That's a good question, I didn't really think about it deeply .. But so for the non-extension dtypes, we also did it for .values / __array__ and to_numpy() (#51082), and so followed along here.

I do think there is value in being consistent in those different ways to get a numpy array from the pandas object. So could also ask, why not for to_numpy()? And then compared to .values, to_numpy() actually gives you more control with the ability to ask for a copy.
(in practice the implementation of __array__ and to_numpy() are also quite overlapping for the EAs.

[jbrockmendel]

So could also ask, why not for to_numpy()?

I don't feel strongly about this, but asked in the first place because it seems most of the code complexity in this PR is driven by to_numpy changes. Without that, most of this is just boilerplate edits to __getitem__ methods.

The main reason i can think of to treat to_numpy different from .array and .values is that it has an explicit copy keyword. With copy=False, the user ideally understands that they are getting a view on existing data.

[jorisvandenbossche]

asked in the first place because it seems most of the code complexity in this PR is driven by to_numpy changes.

Looking at the diff again, I think it is a bit 50/50 between to_numpy() and __array__. But to_numpy() also reuses the result from __array__ in some cases, so if we would then want to have to_numpy() consistently not return readonly data, that would also requires some changes in to_numpy(). So regarding the implementation, not entirely sure this would be a lot simpler (but didn't look in detail).

The main reason i can think of to treat to_numpy different from .array and .values is that it has an explicit copy keyword. With copy=False, the user ideally understands that they are getting a view on existing data.

Yeah, we could potentially also make the default of copy to be None instead of False, with the same meaning (i.e. avoid a copy if possible), and so then if someone explicitly passes copy=False, then we wouldn't set the readonly flag.

From previous discussions (maybe #52823), I seem to remember that we at some point did bring up whether it would be worth having a keyword to control this behaviour, i.e. so there would be a way that you could ask for a numpy array that was guaranteed to be mutable. Of course you could do to_numpy(copy=True) which also guarantees that, but that doesn't cover the case where you want to get the data zero-copy if possible, and you know that mutating it is fine (for example because the holding dataframe or series is dismissed after converting).
At the moment, the documentation (https://pandas.pydata.org/docs/dev/user_guide/copy_on_write.html#read-only-numpy-arrays) suggests to manually reset the readonly flag:

arr = ser.to_numpy()
arr.flags.writeable = True

instead of adding a keyword like arr = ser.to_numpy(ensure_writable=True). But so in theory copy=False could also cover that.

(but this is probably a discussion for #52823)

[jorisvandenbossche]

Are there any other public APIs with inplace keywords (that actually modify inplace) we should test a read only EA with?

Good point to verify. For the EA class itself, it seems we have some methods that can act inplace:

• EA.fillna() if copy=False. For the default base impl, this uses self[:] and then normal setitem, so depending on the point below about getitem, this will be covered. But I can add a specific test to ensure we do this consistently also for other EAs which might have a custom implementation of fillna
• __getitem__ with a slice returns a view, and should thus keep the read-only flag?
• (EA.insert() does not work inplace)
• EA._putmask() -> goes through setitem

For DataFrame et al, there are indeed some methods that allow inplace modifications, and that gives a problem with underlying read-only arrays, but that is not specific for EAs. That is also an issue already on main for default numpy dtypes if then end up using read-only arrays. This is essentially #62144 but then not specifically for __setiten__ but also for other inplace methods (so let's discuss this there?).

[mroeschke]

cc @jbrockmendel if you want to have another look

[jbrockmendel]

do we expect anyone to override this? i.e. does it need to be a method? or just convenient to put it here?

[jbrockmendel]

i guess this makes it easy for subclass authors to use

[jorisvandenbossche]

We currently don't override it anywhere, I just put it here in the base class because it gets used in the __getitem__ implementation of various subclasses. But so indeed does not need to be a method.

I don't think I expect EA authors to override the method (or if override it, they essentially just define it for their own, it's not that the overriding will influence something else on the base class).
They might want to use it for their own getitem implementation, and at that point it is a bit easier to have it as a method instead of a helper function somewhere. But let's start conservative and not expose it to EA authors (we can always add it later)

[jbrockmendel]

this comment (in a few places) confused me until (i think) i figured out it refers to the keyword in the fillna method. could clarify for future-me

[jorisvandenbossche]

Looking back at it, the first comment above "copy=False keyword is not ignored by SparseArray.fillna" also seems wrong, because SparseArray.fillna just completely ignores copy.

Attempted to clarify this.

[jbrockmendel]

small comments, no objections

[jbrockmendel]

not necessarily for this PR, but should we add an EA attribute specifying whether copy=False is ignored? that way we could avoid overriding tests?

[jorisvandenbossche]

Or could also be an attribute on the Test class

[jbrockmendel]

yah either way it can be saved for a follow-up

[jbrockmendel]

thanks @jorisvandenbossche