DEPR: deprecate Index.holds_integer

[topper-123]

See #50042 (comment)

[jbrockmendel]

@topper-123 relying on this at all for internal purposes is what i dislike. whether its a method vs function is totally irrelevant

[topper-123]

Ok, can you explan what's the issue with this approach? This method was only used in the plotting module, so a very little used function? Should we replace it there with normal dtype checking, i.e. just drop the "mixed-integer" checks?

[jbrockmendel]

this is "values-dependent behavior" which in general we are trying to move away from. it makes it much harder to reason about code. one of the reasons this is little-used is i have been trying to remove usages of it.

i expect most of the relevant cases could just be is_integer_dtype(self.dtype). there would likely be some straggler object-dtype-of-integer cases that would need some deprecation cycle

[topper-123]

Ok, I understand. I'll look into it tomorrow. I've never worked on this part of the code base, hopefully it's not too complex.

[topper-123]

Do we want to just use return self.inferred_type not in ["integer", "mixed-integer"] here instead of the multiliner?

[jbrockmendel]

looks like all our tests pass if we change this whole thing to return self.dtype.kind in ["i", "u"]. I think the thing to do is drop this for now and deprecate it in 2.x and change it to this simpler check in 3.0.

[topper-123]

I'm not sure I follow how could deprecate this, as this is an internal attribute, and I don't think this changes behavior (though I'm not 100 % sure there isn't an edge case).

Are you saying only use return self.inferred_type not in ["integer", "mixed-integer"] here and drop the if/elif/else?

[jbrockmendel]

though I'm not 100 % sure there isn't an edge case

Under my proposal, pd.Index([1, 2], dtype=object).holds_integer() would change in the future.

The only places where we use holds_integer are in _should_fallback_to_positional (which I expect to get rid of in 3.0) and plotting code. It would require some actual effort to determine if the suggested change could affect users via the plotting code.

[topper-123]

Concretely in this PR, I git difficulty understanding what you want. Can you say what code you want changed, e.g. should the proposed deprecation of holds_integer be dropped? IMO it does seems like a good idea to deprecate the method...

[jbrockmendel]

in this PR i would suggest

def _holds_integer(self) -> bool:
     return self.inferred_type not in ["integer", "mixed-integer"] 

def holds_integer(self) -> bool:
    warngs.warn("holds_integer is deprecated [...]", FutureWarning)
    return self._holds_integer()

[topper-123]

Ok, nop problem, I've just updated the PR.

[topper-123]

@jbrockmendel , any comment?

[jbrockmendel]

i dont think these are exactly the same. _should_fallback_to_positional is overriden by Index subclasses

[topper-123]

Hmm, my instinct is that the old code easily give wrong result by giving an integer to an index that has _should_fallback_to_positional set to False and it should instead have been crystal clear if x/y are meant for .loc/.iloc instead of this in thiscode section. But this is difficult to prove and I'm not very familiar with the plotting code, so I've changed to use _holds_integer.

[jbrockmendel]

i agree the current code is sketchy. hopefully we'll figure out what the original intent was and can change it to something more like is_integer_dtype(data.columns.dtype)

[topper-123]

The ARM failure looks unrelated.

[topper-123]

I've changed this to use Index._holds_integer.

[topper-123]

Ping.

[jbrockmendel]

LGTM. Holding off on merging until the existing deprecations are all enforced for 2.0.

[topper-123]

👍 .

[topper-123]

I've updated this PR after it was decided that it is ok now to merge deprecation PRs.

The code checks errors are unrelated to this PR.

[topper-123]

I’ve rerun the failed tests and everything is green now.

[phofl]

thx @topper-123