CLN: tighten Exception catching in indexing.py by jbrockmendel · Pull Request #29305 · pandas-dev/pandas (original) (raw)
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation15 Commits5 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
get rid of is_lexsorted_for_tuple while we're at it
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 |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned a few lines above, this is apparently for compatibility with MultiIndex. That is no longer needed?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think its a big deal to remove this method. Could have a note about it I guess.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we then remove it from MultiIndex as well?
It is not overridden by MultiIndex
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Are exceptions simply being handled more specifically now? Or are they be handled higher up in the stack now?
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.
Reksbril pushed a commit to Reksbril/pandas that referenced this pull request
proost pushed a commit to proost/pandas that referenced this pull request
proost pushed a commit to proost/pandas that referenced this pull request
Labels
Related to indexing on series/frames, not to indexes themselves