Skip to content

CLN: tighten Exception catching in indexing.py #29305

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Nov 3, 2019
Merged

CLN: tighten Exception catching in indexing.py #29305

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3da777a
CLN: tighten Exception catching in indexing.py
jbrockmendel Oct 31, 2019
41e09d4
Merge branch 'master' of https://github.com/pandas-dev/pandas into fa…
jbrockmendel Oct 31, 2019
6b89292
blackify
jbrockmendel Oct 31, 2019
3b8632c
Merge branch 'master' of https://github.com/pandas-dev/pandas into fa…
jbrockmendel Nov 2, 2019
9a0a863
whatsnew
jbrockmendel Nov 2, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion doc/source/reference/indexing.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,6 @@ Compatibility with MultiIndex
:toctree: api/

Index.set_names
Index.is_lexsorted_for_tuple
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member

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)

Copy link
Contributor

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.

Copy link
Member

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.

Copy link
Member Author

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

Copy link
Member

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)

Copy link
Member Author

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?

Copy link
Member

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!

Index.droplevel

Missing values
Expand Down
1 change: 1 addition & 0 deletions doc/source/whatsnew/v1.0.0.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,6 +256,7 @@ or ``matplotlib.Axes.plot``. See :ref:`plotting.formatters` for more.
- Ability to read pickles containing :class:`Categorical` instances created with pre-0.16 version of pandas has been removed (:issue:`27538`)
- Removed the previously deprecated ``reduce`` and ``broadcast`` arguments from :meth:`DataFrame.apply` (:issue:`18577`)
- Removed the previously deprecated ``assert_raises_regex`` function in ``pandas.util.testing`` (:issue:`29174`)
- Removed :meth:`Index.is_lexsorted_for_tuple` (:issue:`29305`)
-

.. _whatsnew_1000.performance:
Expand Down
3 changes: 0 additions & 3 deletions pandas/core/indexes/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1726,9 +1726,6 @@ def _is_strictly_monotonic_decreasing(self):
"""
return self.is_unique and self.is_monotonic_decreasing

def is_lexsorted_for_tuple(self, tup):
return True

@cache_readonly
def is_unique(self):
"""
Expand Down
18 changes: 1 addition & 17 deletions pandas/core/indexing.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,6 @@ class IndexingError(Exception):

class _NDFrameIndexer(_NDFrameIndexerBase):
_valid_types = None # type: str
_exception = Exception
axis = None

def __call__(self, axis=None):
Expand Down Expand Up @@ -881,14 +880,6 @@ def _handle_lowerdim_multi_index_axis0(self, tup: Tuple):
# else IndexingError will be raised
if len(tup) <= self.obj.index.nlevels and len(tup) > self.ndim:
raise ek
except Exception as e1:
if isinstance(tup[0], (slice, Index)):
raise IndexingError("Handle elsewhere")

# raise the error if we are not sorted
ax0 = self.obj._get_axis(0)
if not ax0.is_lexsorted_for_tuple(tup):
raise e1

return None

Expand Down Expand Up @@ -1385,8 +1376,6 @@ def _convert_for_reindex(self, key, axis: int):


class _LocationIndexer(_NDFrameIndexer):
_exception = Exception

def __getitem__(self, key):
if type(key) is tuple:
key = tuple(com.apply_if_callable(x, self.obj) for x in key)
Expand Down Expand Up @@ -1417,10 +1406,7 @@ def _getbool_axis(self, key, axis: int):
labels = self.obj._get_axis(axis)
key = check_bool_indexer(labels, key)
inds = key.nonzero()[0]
try:
return self.obj.take(inds, axis=axis)
except Exception as detail:
raise self._exception(detail)
return self.obj.take(inds, axis=axis)

def _get_slice_axis(self, slice_obj: slice, axis: int):
""" this is pretty simple as we just have to deal with labels """
Expand Down Expand Up @@ -1685,7 +1671,6 @@ class _LocIndexer(_LocationIndexer):
"endpoints included! Can be slices of integers if the "
"index is integers), listlike of labels, boolean"
)
_exception = KeyError

@Appender(_NDFrameIndexer._validate_key.__doc__)
def _validate_key(self, key, axis: int):
Expand Down Expand Up @@ -1970,7 +1955,6 @@ class _iLocIndexer(_LocationIndexer):
"integer, integer slice (START point is INCLUDED, END "
"point is EXCLUDED), listlike of integers, boolean array"
)
_exception = IndexError
_get_slice_axis = _NDFrameIndexer._get_slice_axis

def _validate_key(self, key, axis: int):
Expand Down