pybind: Add __hash__ to all binding types #627
Conversation
In Python 3, defining __eq__ without __hash__ sets __hash__ = None, making the type unhashable. All seven affected types raised: TypeError: unhashable type: 's2geometry_bindings.S1Angle' Adds __hash__ to S1Angle, S1Interval, R1Interval, R2Point, R2Rect, S2LatLng, and S2Point using absl::Hash on their underlying values.
deustis
changed the title
jmr
left a comment
jmr
left a comment
There was a problem hiding this comment.
Similar for the rest of the classes.
| return self /= v; | ||
| }, py::arg("v"), "In-place division by scalar") | ||
| .def("__hash__", [](const R2Point& self) { | ||
| return absl::Hash<std::pair<double, double>>()( |
There was a problem hiding this comment.
There's no AbslHashValue for vector? This should be absl::HashOf(self), I think.
There was a problem hiding this comment.
Posted by Claude: R2Point is BasicVector<double, 2> which already has AbslHashValue defined in vector.h, so absl::HashOf(self) works there. For the types that were missing it (R1Interval, S1Angle, S1Interval, R2Rect), I've added AbslHashValue free functions to their headers and updated all binding files to use absl::HashOf(self) uniformly.
|
Updated The remaining four — |
These types already define AbslHashValue, so absl::HashOf(self) is more idiomatic than manually extracting and hashing fields.
| .def(py::self != py::self, "Return true if two intervals do not contain the same set of points") | ||
| .def("__hash__", [](const R1Interval& self) { | ||
| return absl::Hash<std::pair<double, double>>()( | ||
| std::make_pair(self.lo(), self.hi())); |
There was a problem hiding this comment.
Add AbslHashValue for R1Interval and other types missing it
There was a problem hiding this comment.
Done: added AbslHashValue free functions to r1interval.h, s1angle.h, s1interval.h, and r2rect.h, then updated all four binding files to use absl::HashOf(self) uniformly.
Use AbslHashValue free functions in the respective headers so that the Python bindings can call absl::HashOf(self) uniformly instead of manually assembling a std::pair/tuple over the component fields.
…2Rect Uses absl::VerifyTypeImplementsAbslHashCorrectly to confirm the newly added AbslHashValue functions satisfy the hash contract.
| .def("__truediv__", [](const S1Angle& a, const S1Angle& b) -> double { | ||
| return a / b; | ||
| }, py::arg("other"), "Divide two angles, returning a scalar ratio") | ||
| .def(py::self += py::self, "In-place addition") |
There was a problem hiding this comment.
Does this need to be merged/rebased? it looks like more than hash
There was a problem hiding this comment.
This was intentional, based on the other PR thread:
#626 (comment)
We decided to remove bindings for in-place operators because they make hashing unstable. I rolled those changes into this PR (updated the PR description just now).
There was a problem hiding this comment.
Please keep them separate. This PR is "add hash".
There was a problem hiding this comment.
Done! Moved the in-place operator removal to #629
deustis
force-pushed
the
deustis/fix_hash_bindings
branch
from
a2e57e6 to
040dd76
Compare
2 participants
In Python 3, defining
__eq__without__hash__implicitly sets__hash__ = None, making the type unhashable. All seven affected types raised:Adds
__hash__toS1Angle,S1Interval,R1Interval,R2Point,R2Rect,S2LatLng, andS2Pointusingabsl::Hashon their underlying values.