-
-
Notifications
You must be signed in to change notification settings - Fork 33.9k
gh-143831: Compare cells by identity in forward references #143848
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?
Conversation
| self.__forward_is_class__, | ||
| tuple(sorted(self.__cell__.items())) if isinstance(self.__cell__, dict) else self.__cell__, | ||
| ( # cells are mutable and not hashable as well | ||
| tuple(sorted([(name, id(cell)) for name, cell in self.__cell__.items()])) |
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.
@johnslavik Do we need the list comprehension here? We could pass a generator directly to sorted() for slightly better efficiency:
tuple(sorted((name, id(cell)) for name, cell in self.__cell__.items()))Also, is there any way to avoid sorting altogether without compromising the stability or correctness of the hash?
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.
Do we need the list comprehension here? We could pass a generator directly to sorted() for slightly better efficiency:
tuple(sorted((name, id(cell)) for name, cell in self.__cell__.items()))
This is not necessarily more efficient.
List comprehensions are inlined since PEP 709. I've used a list comprehension on purpose here because cell dicts would never be big -- walking a new transient generator on small data is more expensive than computing an eager list. See also #143825 (comment).
Also, is there any way to avoid sorting altogether without compromising the stability or correctness of the hash?
How does sorting here compromise stability or correctness?
This code was meant to sort cell dicts. Although this sounds like an unlikely situation, I think it makes sense for hashes of two different cell dicts that happen to point to identical cells to be the same.
This comment was marked as resolved.
This comment was marked as resolved.
|
Thank you for your interest in this! I encourage you to open a new issue if you think like the performance of Regarding why I solved it this way -- I just went with what the original intent was but with a corrected assumption about the hashability of cells. An alternative approach to hashing a sorted tuple of names -> cell ids is to simply return an identity of |
Cells are mutable and, in regular comparison, they are equal when their contents are equal (or if both are empty).
Two different cells should not be considered equal by value in forward references, though.
If there are two separate cells, they either (1) target different variables or (2) belong to different scopes, so two separate forward references storing two separate cells are conceptually unequal even if these cells happen to be equal at a given point in time. Mutability, essentially.
I'm still working on tests.
TypeError: unhashable type: 'cell'#143831