CLN: tighten Exception catching in indexing.py #29305
Conversation
gfyoung
added
Clean
Indexing
|
Are exceptions simply being handled more specifically now? Or are they be handled higher up in the stack now? |
| @@ -93,7 +93,6 @@ Compatibility with MultiIndex | |||
| :toctree: api/ | |||
|
|
|||
| Index.set_names | |||
| Index.is_lexsorted_for_tuple | |||
There was a problem hiding this comment.
As mentioned a few lines above, this is apparently for compatibility with MultiIndex. That is no longer needed?
There was a problem hiding this comment.
is_lexsorted_for_tuple is used in only one place (deleted by this PR), is not overriden by MultiIndex, and always returns True. It is no longer needed.
There was a problem hiding this comment.
It is not needed by pandas maybe, but it is part of the public API (to make the API of Index and MultiIndex more alike)
There was a problem hiding this comment.
I don't think its a big deal to remove this method. Could have a note about it I guess.
There was a problem hiding this comment.
It was added before to have consistent methods between Index and MultiIndex. Should we then remove it from MultiIndex as well?
To be clear: I don't care about this specific method, it's probably not used much. But we did add other methods to the Index API just to make it easier to have code that works for both index and multi-index. So just wondering why that wouldn't be the case here.
There was a problem hiding this comment.
Should we then remove it from MultiIndex as well?
It is not overridden by MultiIndex
There was a problem hiding this comment.
It is not overridden by MultiIndex
That's not my point. What I am trying to say is: in the past, we tried to keep a consistent api between Index / MultiIndex. So why not do that here?(either keep both, or remove both)
There was a problem hiding this comment.
I'm definitely confused. If this PR is merged, then there will be no is_lexsorted_for_tuple on any class. Which should classify as "remove both", shouldn't it?
There was a problem hiding this comment.
Sorry, I mis-interpreted your "this is not overriden". I was only thinking about the implications for the user (the fact that it is overriden or not does not necessarily mean this method was not there for API compatibility), but of course that means you also removed the method for MultiIndex. So fine with this!
|
Echoing @gfyoung's question, it would be helpful if you add some explanation why you are removing / changing things in your PRs. |
|
lgtm. |
They are no longer reached. AFAICT this is leftover from Panel indexing |
|
can you add a whatsnew note, technically this was public api; ping on green. |
|
ping |
get rid of is_lexsorted_for_tuple while we're at it