BUG: isin incorrectly casting ints to datetimes

[jbrockmendel]

• closes BUG: isin - dtype conversions (Timestamp vs ints) #36621
• tests added / passed
• passes black pandas
• passes git diff upstream/master -u -- "*.py" | flake8 --diff
• whatsnew entry

De-duplication in follow-up.

[jreback]

this going to need perf tests as this is very sensitive

also i am surprised this is needed at all where you did it

[jreback]

are you sure?

ensure_data and below does this

[jbrockmendel]

the way we call ensure_data ends up casting dt64 to int64

[jreback]

why can't you fix it in _ensure_data then, I am not averse to dispatching to the EA itself, which is what we are already doing for Categorical, or is that the plan. If that is the plan, then I would think you could actually remove all off this and simply dispatch (e.g. most of this logic then would actually be handled by PandasArray or the EA as appropriate).

[jbrockmendel]

There are a bunch of GH issues about isin (added a new label yesterday), many of which are about unwanted casting. I haven't looked at them all closely tet.

why can't you fix it in _ensure_data then

Even if we fix that (which yes, i'll take a look, but probably in a separate PR), we still need to dispatch to the (Period|Timedelta|Datetime)Index to get correct casting of the other (assuming we want algos.isin(dtlike, other) to match dtlike.isin(other), which I for one do)

am not averse to dispatching to the EA itself, which is what we are already doing for Categorical, or is that the plan

ATM Categorical has isin but thats it. #20617 suggests adding isin to EA, but thats definitely outside the scope of this PR.

[jbrockmendel]

this going to need perf tests as this is very sensitive

will run the appropriate asvs

[jbrockmendel]

actually, i think this can be simplified to just use _from_sequence following #37179

[jreback]

why can't you fix it in _ensure_data then, I am not averse to dispatching to the EA itself, which is what we are already doing for Categorical, or is that the plan. If that is the plan, then I would think you could actually remove all off this and simply dispatch (e.g. most of this logic then would actually be handled by PandasArray or the EA as appropriate).

[jbrockmendel]

gentle ping. id like to see if we can share some code between this and equals

[jbrockmendel]

Found we don't have any asvs that hit this, so just added a few. No change for cases with matching dtype, nice pickup for others:

       before           after         ratio
     [f823a859]       [9758b021]
     <ref-dtlike-indexers>       <bug-dti-isin>
-        691±30μs          143±3μs     0.21  series_methods.IsInDatetime64.time_isin_empty
-     1.23±0.08ms          141±8μs     0.11  series_methods.IsInDatetime64.time_isin_mismatched_dtype

SOME BENCHMARKS HAVE CHANGED SIGNIFICANTLY.
PERFORMANCE INCREASED.

[jreback]

I guess as a short term measure this is ok as you fixed some bugs. the problem here is that you are bouncing between algos and the indexes, where everything should be in algos. this feels like a step backwards, if its tactical, then ok.

[jbrockmendel]

the problem here is that you are bouncing between algos and the indexes, where everything should be in algos

its not unusual for core.algorithms functions to call EA methods. There's no way the casting logic belongs in core.algorithms.

[jreback]

the problem here is that you are bouncing between algos and the indexes, where everything should be in algos

its not unusual for core.algorithms functions to call EA methods. There's no way the casting logic belongs in core.algorithms.

that's not what's happening here, these are calling Index methods. This seems quite unusual and a very odd depenency.
Why do you not simply move the logic in DTI.isin back to algorithms? what's the point of adding this depenedecy.?

[jbrockmendel]

that's not what's happening here, these are calling Index methods

Good point, my mistake. At some point this should be consolidated in the EA. IIRC theres an issue about adding isin to the interface.

Why do you not simply move the logic in DTI.isin back to algorithms?

because the relevant logic is casting logic that belongs in the DTA. My current thought is that once we get the _from_sequence-strictness PR in we'll be able to de-duplicate some logic between isin+equals using that.

[jbrockmendel]

The new hashtables have me looking at algorithms._ensure_data, and I'm pretty confident that this will allow us to simplify it quite a bit

[jreback]

ok fair