BUG: Make ExtensionArray:_can_hold_na a class attribute

[Dr-Irv]

• closes ExtensionArray._can_hold_na needs to be a class attribute #20761
• tests added / passed
  • pandas/tests/extension/decimal/test_nona.py
    • This tests an example where _can_hold_na==False
• passes git diff upstream/master -u -- "*.py" | flake8 --diff
• whatsnew entry
  • Not included as it is a fix to the new ExtensionArray interface

Supersedes #20801 by removing ops functionality from there.

[jorisvandenbossche]

Question is what this should do if _can_hold_na=False ? And in general with -1 indexers (eg reindex, concat with realignment, ...)
I suppose to raise an error? Or coerce to object?

[Dr-Irv]

Isn't that a design decision that sits above this particular test code? Or am I missing something?

[jorisvandenbossche]

Yes, maybe, but then I am just wondering what this design decision is (and we should also test that here).

As far as I know, this is rather new terrain (in pandas we only have ints, and they are upcasted to float), so it might be we still need to discuss/decide on this.

[jorisvandenbossche]

We also have a BoolBlock that cannot hold NaNs, so a boolean Series gets converted to object Series when NaNs are introduced:

In [49]: pd.Series([True, False])
Out[49]: 
0     True
1    False
dtype: bool

In [50]: pd.Series([True, False]).reindex([1, 2])
Out[50]: 
1    False
2      NaN
dtype: object

So this is some prior art

[Dr-Irv]

So by the statement "we still need to discuss/decide on this", I presume that the core dev team will make that call. Right? (FWIW, I think I may use _can_hold_na=True, and since I'm storing objects, I'll just use NaN to be the na_value that is then used in reindex, shift, etc.), and I'd want the type of the resulting array to remain in my subclass of ExtensionArray

[jorisvandenbossche]

So by the statement "we still need to discuss/decide on this", I presume that the core dev team will make that call. Right?

No, this is an open discussion, and your voice in this discussion is important since you are actually one of the first people trying to use this (both Tom and I do not use _can_hold_na=False in our applications).

But if you would also not use this, we should maybe consider removing the feature for now (meaning this will always be True) until somebody comes up with a good use case.

[Dr-Irv]

Well, I've come full circle on this. In my case, I thought that I would not want NA in my arrays. My objects stored in the arrays have no NA value that makes sense. And then when you brought up reindex as an example, I realized that I would use things like shift and would want the NA to be filled in, but keeping the ExtensionArray class as the holder, meaning that I need _can_hold_na as True. I'd want ExtensionArray as holding it so that arithmetic would work, as coercing to an object array would mess up the arithmetic. So I'd have no problem if we dropped _can_hold_na from the ExtensionArray design, and tell people that they must define a _na_value(), which is then used as the "fill in" when we do things like reindex and shift. And because there are pandas operations that need some NA value for fill in purposes, requiring it on ExtensionArray shouldn't be an issue (IMHO).

If we agree that getting rid of _can_hold_na is the thing to do, I'll modify this PR to totally get rid of it (and the associated tests).

[jorisvandenbossche]

Those constructor tests are not specific to the "nona", so I would move them to a separate file

[Dr-Irv]

I just copied them from test_decimal.py and changed the array type. I guess we need something in the base tests. I'll look at that.

[jorisvandenbossche]

This is maybe something we need to discuss, as in the take PR Tom was adding an _na_value to the dtype itself. We should see how this interacts

[jorisvandenbossche]

Let's put the discussion here to not hide it in a review comment:

So I questioned what should happen when NAs are introduced (eg reindex) in ExtensionArray that has _can_hold_na=False.
I suppose to raise an error? Or coerce to object?

Currently, in pandas we have ints, and they are upcasted to float. We also have a BoolBlock that cannot hold NaNs, so a boolean Series gets converted to object Series when NaNs are introduced:

In [49]: pd.Series([True, False])
Out[49]: 
0     True
1    False
dtype: bool

In [50]: pd.Series([True, False]).reindex([1, 2])
Out[50]: 
1    False
2      NaN
dtype: object

So this is some prior art. But it would also mean you loose your ExtensionArray/ExtensionDtype information.

So remark: if we currenlty don't have users of _can_hold_na=False, we should maybe consider removing the feature for now (meaning this will always be True) until somebody comes up with a good use case.

@Dr-Irv :

Well, I've come full circle on this. In my case, I thought that I would not want NA in my arrays. My objects stored in the arrays have no NA value that makes sense. And then when you brought up reindex as an example, I realized that I would use things like shift and would want the NA to be filled in, but keeping the ExtensionArray class as the holder, meaning that I need _can_hold_na as True. I'd want ExtensionArray as holding it so that arithmetic would work, as coercing to an object array would mess up the arithmetic. So I'd have no problem if we dropped _can_hold_na from the ExtensionArray design, and tell people that they must define a _na_value(), which is then used as the "fill in" when we do things like reindex and shift. And because there are pandas operations that need some NA value for fill in purposes, requiring it on ExtensionArray shouldn't be an issue (IMHO).

If we agree that getting rid of _can_hold_na is the thing to do, I'll modify this PR to totally get rid of it (and the associated tests).

cc @TomAugspurger @jreback

[jreback]

i would not add API unless we have a need for it
so yes would remove until / until is necessary

[Dr-Irv]

Closing due to new PR #20819