Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
CI: Test Python 3.12 #53743
CI: Test Python 3.12 #53743
Changes from 22 commits
ff98491
fcbe1ed
dd4a488
c809bcd
cf7e272
8c7aea2
dc88fa8
d2b3868
5814bcc
7be7ea9
e9c0ed4
f735b8c
80d31e1
b8f351d
7cbfd7c
e507d45
90dbdeb
5e421f3
0e04fd2
b5ae510
b1182ac
ddae8f8
64e12a9
bc64b47
2c1755e
5dfc14b
a1bd210
298f31b
88a0cb8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
the linked issue is now closed; can this be reenabled?
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.
Matt enabled this again a while back, I think.
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.
what cases get here?
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.
Probably this one
https://github.com/pandas-dev/pandas/pull/53743/files#diff-c34a28314fc8cb12f0d2aa710f1c15f06cdfe3e48f03e658f01f99a43d4f5d09R3793-R3797
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 does changing the order matter? the current order is pretty fine-tuned for perf
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.
We need to check for slices in iterators by iterating through them, since they aren't allowed as hash keys.
(It doesn't make sense to have slices as keys in an Index and it breaks way too many things).
This exhausts the generator.
IIUC, this should only lower perf for generators?
(at least the
is_iterator
docstring only mentions that it will only return True for generators not list and co.)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.
very late review comment: would it make sense to make is_hashable_non_slice or something? it wouldn't surprise me if many places that use is_hashable current assume non-slice but didn't get updated by this PR
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.
Yeah sure, I just updated the places that broke in the tests.
Is there a case where we would actually want
is_hashable
for a slice to equal True, though?(I was thinking it might be cleaner to make
is_hashable
always return False for slices. This would be a breaking API change, though).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.
i think having is_hashable be anything other than a try/except around hash would cause problems. im suggesting a new function to de-duplicate the hashable-but-not-slice checks this introduces
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.
Made an issue #55152.
I don't have the same time that I had in summer to work on pandas, but I'll try to have a look (no promises, though).