API/DEPR: Remove +/- as setops for Index (GH8227)

[jorisvandenbossche]

xref #13777, deprecations put in place in #8227

• tests added / passed
• passes git diff upstream/master | flake8 --diff
• whatsnew entry

[jreback]

I think this is not defined in super? so this should raise NotImplementedError I think?

[jorisvandenbossche]

yep, this is something that I still have to handle.

The problem is that sub/add are added for subclasses in either _add_numericlike_set_methods_disabled or _add_numeric_methods or both ovenwritten (always handled both), but in the base class we only want __add__ and not __sub__

If I leave it in here, I think it should rather raise a TypeError? (for usage of base Index class)

[jreback]

yeah TypeError is prob ok too

the only reason to raise NotImplemmeted is that then subclasses could override - but we don't want that

[jreback]

this needs to remain I think? (the isinstance part)

[jorisvandenbossche]

Why the isinstance part? (we don't only allow to add an Index, but also a scalar or list/array)

[jreback]

I think this really should use _ensure_index I think

[jorisvandenbossche]

But scalars should also work?

In [13]: pd.Index(['a', 'b']) + '1'
Out[13]: Index(['a1', 'b1'], dtype='object')

BTW, on numeric indexes, using lists is not allowed:

In [12]: pd.Index([1, 2]) + [10, 20]
TypeError: can only perform ops with scalar values

In [13]: pd.Index([1, 2]) + np.array([10, 20])
Out[13]: Int64Index([11, 22], dtype='int64')

while on base Index it is currently allowed:

In [15]: pd.Index(['a', 'b']) + ['c', 'd']
Out[15]: Index(['ac', 'bd'], dtype='object')

Unifying that would also be nice. Using _ensure_index in this case would still allow lists

[jreback]

I think we are expanding the api w/o any testing of __add__. Its ok, but should create an issue to add tests and/or limit it. (so ideally we don't expand it at all for Index and add back if needed later), esp NOT for MultiIndex and Categorical (I think you just renamed, but just to be sure).

[jorisvandenbossche]

Yes, for MultiIndex/Categorical nothing should be changed. I just renamed the method to disable add/sub for those, as there are no longer set ops.

For Index, I agree we should be more careful on what to allow, but I think that this is already the case in current master.
The 'old' __add__ did basically:

def __add__(self, other):
    if isinstance(other, Index):
        self.union(other)
    return Index(self.values + other)

So by removing the if instance part I am not broading the scope, previously everything that was not an Index was already handled to do addition.

But, as I said, I agree we should be more careful / do more careful testing (eg Int64Index does not allow to add with lists, base Index does).
I will see if I do that here, or leave it for another issue.

[jreback]

I would be a break in here (e.g. where it currently calls .union) and see if we actually have test code that hits this. It obviously works, so prob ok to remove, just checking.

[jorisvandenbossche]

Not sure I understand this comment. We had indeed tests that tested the union behaviour of + for Index, and I changed this test to check for concatenation. If there are other tests that would hit this path, they should now fail given that I removed it.

[jreback]

?

[codecov-io]

Current coverage is 85.25% (diff: 100%)

Merging #14127 into master will decrease coverage by <.01%

@@             master     #14127   diff @@
==========================================
  Files           139        139          
  Lines         50501      50495     -6   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits          43058      43051     -7   
- Misses         7443       7444     +1   
  Partials          0          0          

Powered by Codecov. Last update 4488f18...dbcc655

[jreback]

@jorisvandenbossche we should do this for the RC as well.

[jorisvandenbossche]

@jreback yep, as it is quite a substantial change (for people that ignored the warnings)

Is there a reason that we do not allow numeric ops for the base Index class? (eg pd.Index(['a', 'b'] could have a clear meaning, just letting the objects determine whether an operation can be handled)

Because eg the __add__ in the Index class could benefit from the machinery in _add_numeric_methods_binary (eg to ensure attributes like name are passed correctly. Something that is also not correct on current master)

[jreback]

I think we don't allow numeric operations on Index as only possible one is add for string concat
nothing else makes sense

in general these r not well defined so no reason to allow

[shoyer]

I think we should let the objects in the Index decide whether the numeric operation is valid or not. This is the approach NumPy uses for object dtype. For example, one might put decimal objects in a pandas.Index.

[jreback]

the numpy approach leads to lots of bugs and edge cases - constraining things somewhat to not allowing everything on everything is much more user friendly

decimal objects in an Index are a disaster - should not be encouraged

[jreback]

I would be more specific that 'when possible' (maybe say for numeric & datetimelike)

[jorisvandenbossche]

datetimelike not yet (DatetimeIndex - DatetimeIndex does still set op, but this one is a bit more work to remove).

And + also works for strings, so not only numeric. Further, for numeric indices, + and - did already do numeric ops and not setops .. (to make it a bit more complex :-))

So I did not directly come up with a more accurate but simple wording, but if you have a suggestion all ears!

[jreback]

make this clear that this ONLY applies to Index itself and not-subclasses (maybe show that numeric/datetimelike ops are unchanged)

[jorisvandenbossche]

yes, will try to clarify

[jreback]

This behaviour was deprecated since 0.15.0 (already is redundant)

[jreback]

@jorisvandenbossche some small comments, but otherwise lgtm. merge when ready.