REF: implement _should_compare

[jbrockmendel]

• closes #xxxx
• tests added / passed
• passes black pandas
• passes git diff upstream/master -u -- "*.py" | flake8 --diff
• whatsnew entry

Index._is_comparable_dtype is limited because it misses a few cases: object dtype, and sub-dtypes within categorical. This implements Indes._should_compare, which handles those correctly. It then implements _get_indexer_non_comparable for cases in which we short-circuit non-comparable dtypes.

As a proof of concept, this then uses _should_compare and _get_indexer_non_comparable for PeriodIndex.get_indexer.

The behavior change, which this tests, is one that IIUC is a bug. That is, when we do get_indexer with a method and non-comparable dtypes, we should raise instead of return all minus-ones.

If implemented, we'll be able to use _should_compare to simplify all of the get_indexer, get_indexer_non_unique, and set op methods.

[jbrockmendel]

@jreback theres some ambiguity as to what we're supposed to be returning here. ATM in Index.get_indexer_non_unique we are returning two ndarrays of minus-ones, but in IntervalIndex the second one is an np.arange(len(target))

The other inconsistency is that ATM PeriodIndex.get_indexer defines no_matches = -1 * np.ones(self.shape, dtype=np.intp) whereas i expected it to use target.shape

Can you clear this up for me?

[jbrockmendel]

looking at this more, i increasingly think the second one should be np.arange(target.shape, dtype=np.intp)

[jreback]

i don't think this method should raise.

this is a pretty big change from status quo

[jbrockmendel]

i don't think this method should raise.

this is a pretty big change from status quo

I stumbled onto this in part because of this test:

def test_reindex_datetimeindexes_tz_naive_and_aware():
    # GH 8306
    idx = date_range("20131101", tz="America/Chicago", periods=7)
    newidx = date_range("20131103", periods=10, freq="H")
    s = Series(range(7), index=idx)
    msg = "Cannot compare tz-naive and tz-aware timestamps"
    with pytest.raises(TypeError, match=msg):
        s.reindex(newidx, method="ffill")

and i figured that the way we treat periods with mismatched freq is almost identical to how we treat tzawareness-compat. Or is dt64tz special in this case?

I'd be OK with not-raising in _get_indexer_non_comparable, just want to get this behavior consistent, bc ATM we're all over the place.

[jbrockmendel]

@jreback notice in these cases we are currently raising in master bc the scalar comparisons raise

[jreback]

is this new?

get_indexer is not super public but i believe it will never raise

[jbrockmendel]

I think you're right that get_indexer with method=None should never raise (maybe with tzawareness corner cases), but with method="ffill" the following raises on master:

dti = pd.date_range("2016-01-01", periods=3)
rng = pd.Index(range(5))

>>> dti.get_indexer(rng, method="ffill")
TypeError: '<' not supported between instances of 'int' and 'Timestamp'

[jbrockmendel]

xref #36320

[jbrockmendel]

So I'm increasingly convinced that the current PeriodIndex.get_indexer behavior with method != None is wrong:

dti = pd.date_range("2016-01-01", periods=3)
pi = dti.to_period("D")
pi2 = dti.to_period("W")

ser = pd.Series(range(3), pi)

>>> ser.reindex(pi2, method="ffill")
2015-12-28/2016-01-03   NaN
2015-12-28/2016-01-03   NaN
2015-12-28/2016-01-03   NaN
Freq: W-SUN, dtype: float64

But pi vs pi2 are uncomparable just like pi vs dti, so we should raise the same way:

>>> ser.reindex(dti, method="ffill")
[...]
  File "pandas/_libs/algos.pyx", line 450, in pandas._libs.algos.pad
    if nleft == 0 or nright == 0 or new[nright - 1] < old[0]:
TypeError: '<' not supported between instances of 'Timestamp' and 'Period'

[jreback]

non_comparable_idx

[jreback]

this should just be a function, no? its also a funny name. I think we have other similar things (e.g. this look like what .to_dense() does but for dtypes and not values).

[jbrockmendel]

updated to _unpack_nested_dtypes function

[jbrockmendel]

alright! get ready to see a bunch of de-duplication and fastpaths coming in.

[jreback]

hit me!