-
-
Notifications
You must be signed in to change notification settings - Fork 19.2k
CLN: Add allow_slice to is_hashable function #62567
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 and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 28 commits
9a1c452
ee2bf48
70a8357
bae4e48
c324360
37fec55
859f2f8
90cb69e
4544845
0e4d675
03c1b22
8ded755
067b830
8fcecee
871d621
d145d0b
2b0ba79
2a6e7a1
7b83c16
0a57d73
c684c43
06af37c
fb3e85f
272a17b
4440d70
fd30506
62793a4
2a3a0a8
1331063
095f473
0c97a82
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -793,8 +793,7 @@ def _get_setitem_indexer(self, key): | |
| if ( | ||
| isinstance(ax, MultiIndex) | ||
| and self.name != "iloc" | ||
| and is_hashable(key) | ||
| and not isinstance(key, slice) | ||
| and is_hashable(key, allow_slice=False) | ||
| ): | ||
| with suppress(KeyError, InvalidIndexError): | ||
| # TypeError e.g. passed a bool | ||
|
|
@@ -1147,8 +1146,7 @@ def _contains_slice(x: object) -> bool: | |
| # This should never be reached, but let's be explicit about it | ||
| raise ValueError("Too many indices") # pragma: no cover | ||
| if all( | ||
| (is_hashable(x) and not _contains_slice(x)) or com.is_null_slice(x) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i think this is the only usage of _contains_slice, so that can be removed? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @LirongMa can you address this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed |
||
| for x in tup | ||
| is_hashable(x, allow_slice=False) or com.is_null_slice(x) for x in tup | ||
| ): | ||
| # GH#10521 Series should reduce MultiIndex dimensions instead of | ||
| # DataFrame, IndexingError is not raised when slice(None,None,None) | ||
|
|
@@ -1511,12 +1509,8 @@ def _convert_to_indexer(self, key, axis: AxisInt): | |
|
|
||
| # Slices are not valid keys passed in by the user, | ||
| # even though they are hashable in Python 3.12 | ||
| contains_slice = False | ||
| if isinstance(key, tuple): | ||
| contains_slice = any(isinstance(v, slice) for v in key) | ||
|
|
||
| if is_scalar(key) or ( | ||
| isinstance(labels, MultiIndex) and is_hashable(key) and not contains_slice | ||
| isinstance(labels, MultiIndex) and is_hashable(key, allow_slice=False) | ||
|
||
| ): | ||
| # Otherwise get_loc will raise InvalidIndexError | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -213,7 +213,7 @@ def __init__( # pyright: ignore[reportInconsistentConstructor] | |
| ) | ||
|
|
||
| try: | ||
| self._book = Workbook(self._handles.handle, **engine_kwargs) # type: ignore[arg-type] | ||
|
||
| self._book = Workbook(self._handles.handle, **engine_kwargs) | ||
| except TypeError: | ||
| self._handles.handle.close() | ||
| raise | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -34,6 +34,7 @@ | |
| missing as libmissing, | ||
| ops as libops, | ||
| ) | ||
| from pandas.compat import PY312 | ||
| from pandas.compat.numpy import np_version_gt2 | ||
| from pandas.errors import Pandas4Warning | ||
|
|
||
|
|
@@ -452,16 +453,55 @@ class UnhashableClass2: | |
| def __hash__(self): | ||
| raise TypeError("Not hashable") | ||
|
|
||
| class HashableSlice: | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. comment that we can get rid of that once we drop py311? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. comments added |
||
| def __init__(self, start, stop, step=None): | ||
| self.slice = slice(start, stop, step) | ||
|
|
||
| def __eq__(self, other): | ||
| return isinstance(other, HashableSlice) and self.slice == other.slice | ||
|
|
||
| def __hash__(self): | ||
| return hash((self.slice.start, self.slice.stop, self.slice.step)) | ||
|
|
||
| def __repr__(self): | ||
| return ( | ||
| f"HashableSlice({self.slice.start}, {self.slice.stop}, " | ||
| f"{self.slice.step})" | ||
| ) | ||
|
|
||
| hashable = (1, 3.14, np.float64(3.14), "a", (), (1,), HashableClass()) | ||
| not_hashable = ([], UnhashableClass1()) | ||
| abc_hashable_not_really_hashable = (([],), UnhashableClass2()) | ||
| hashable_slice = HashableSlice(1, 2) | ||
| tuple_with_slice = (slice(1, 2), 3) | ||
|
|
||
| for i in hashable: | ||
| assert inference.is_hashable(i) | ||
| assert inference.is_hashable(i, allow_slice=True) | ||
| assert inference.is_hashable(i, allow_slice=False) | ||
| for i in not_hashable: | ||
| assert not inference.is_hashable(i) | ||
| assert not inference.is_hashable(i, allow_slice=True) | ||
| assert not inference.is_hashable(i, allow_slice=False) | ||
| for i in abc_hashable_not_really_hashable: | ||
| assert not inference.is_hashable(i) | ||
| assert not inference.is_hashable(i, allow_slice=True) | ||
| assert not inference.is_hashable(i, allow_slice=False) | ||
|
|
||
| assert inference.is_hashable(hashable_slice) | ||
| assert inference.is_hashable(hashable_slice, allow_slice=True) | ||
| assert inference.is_hashable(hashable_slice, allow_slice=False) | ||
|
|
||
| if PY312: | ||
| for obj in [slice(1, 2), tuple_with_slice]: | ||
| assert inference.is_hashable(obj) | ||
| assert inference.is_hashable(obj, allow_slice=True) | ||
| assert not inference.is_hashable(obj, allow_slice=False) | ||
| else: | ||
| for obj in [slice(1, 2), tuple_with_slice]: | ||
| assert not inference.is_hashable(obj) | ||
| assert not inference.is_hashable(obj, allow_slice=True) | ||
| assert not inference.is_hashable(obj, allow_slice=False) | ||
|
|
||
| # numpy.array is no longer collections.abc.Hashable as of | ||
| # https://github.com/numpy/numpy/pull/5326, just test | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is None needed?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
None is a default value. So for exsiting callers, I don't need to set allow_slice. Unless you want me to change all the callers of is_hahsable and set the allow_slice to be True (if they don't have extra checks for slice). Please let me know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just set the default to True.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, set the default to be True now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done