pybind: Add S1ChordAngle bindings#626
Conversation
Binds S1ChordAngle with constructors, factory methods (from_radians, from_degrees, from_e5/6/7, fast_upper_bound_from, from_length2, zero, right, straight, infinity, negative), properties (length2, radians, degrees, e5/6/7), predicates (is_zero, is_negative, is_infinity, is_special, is_valid), geometric ops (to_angle, sin/cos/tan, sin2, successor, predecessor, plus_error, error bounds), arithmetic (+, -, +=, -=) with pre-validation that rejects Negative()/Infinity() operands, comparisons, hash, and __repr__/__str__. Unblocks the deferred distance methods on S2Cell (GetDistance, GetMaxDistance, IsDistanceLess, etc.) which can be added in a follow-up. Stacked on deustis/s2cell_bindings.
- Use S2::IsUnitLength for unit-length validation, matching the C++ DCHECK exactly; add s2pointutil.h include - Drop fast_upper_bound_from, from_length2, from_e5/e6/e7 factory methods - Drop length2 property, sin2, successor, predecessor, plus_error, and error-bound accessor bindings - Update tests accordingly, replacing length2 assertions with radians/degrees/comparisons
- Guard sin/cos/tan against special values (negative/infinity) to prevent DCHECK in C++; add test_sin_cos_tan_special_raises - Guard e5/e6/e7 properties against special values; negative() would silently return a meaningless integer without this guard; add test_e5_e6_e7_special_raises - Rename MaybeThrowSpecialForArithmetic -> MaybeThrowIfSpecial - Update error messages to use Python-style lowercase names (negative(), infinity()) - Switch __hash__ from std::hash<double> to absl::Hash<double>; add absl/hash BUILD dep - Fix test_str and test_repr to assert exact values - Fix test_constructor_from_antipodal_points to use assertEqual(straight()) - Add test_to_angle_special_values and test_str_negative_sentinel
Such as which? And what is "guarded"? Throws an exception? |
|
Specifically, |
| }, py::is_operator(), | ||
| "Subtract two chord angles, clamping the result to [0, Pi].\n\n" | ||
| "Raises ValueError if either operand is Negative() or Infinity().") | ||
| .def("__iadd__", [](S1ChordAngle& self, const S1ChordAngle& other) { |
There was a problem hiding this comment.
I think these should be removed, since they can mutate something in a dict. is that right? there may be other classes with this problem.
There was a problem hiding this comment.
Done.
Removed __iadd__ and __isub__. Without them, +=/-= fall back to __add__/__sub__ + rebinding, so the operations still work but no longer mutate the object in place. Renamed the corresponding tests to test_add_assign/test_sub_assign accordingly.
The same issue applies to the other hashable types in PR #627 (S1Angle, R2Point, S2LatLng, S2Point). I have updated that PR to remove some of the in-place operators and clarify in the README
| with self.assertRaises(ValueError): | ||
| _ = a - s2.S1ChordAngle.negative() | ||
|
|
||
| def test_iadd(self): |
There was a problem hiding this comment.
this should still work, so give it a different name
There was a problem hiding this comment.
Renamed to test_add_assign and test_sub_assign.
In-place mutation operators are unsafe on hashable types: mutating a dict key changes its hash, making the entry unreachable. Without __iadd__/__isub__, Python falls back to __add__/__sub__ + rebinding, so a += b still works but creates a new object instead of mutating. Rename test_iadd/test_isub to test_add_assign/test_sub_assign to reflect that += / -= delegate to __add__ / __sub__.
2 participants
Add pybind11 bindings for S1ChordAngle.
Includes constructors, factory methods (from_radians, from_degrees, zero, right, straight, infinity, negative), properties (radians, degrees, e5/e6/e7), predicates (is_zero, is_negative, is_infinity, is_special, is_valid), geometric operations (to_angle, sin, cos, tan), arithmetic operators (+, -, +=, -=), comparisons, hash, and repr/str.
The
e5/e6/e7properties,sin/cos/tan, and arithmetic operators (+,-,+=,-=) raiseValueErrorfor special values (negative()orinfinity()). Operations such as comparisons,radians,degrees,to_angle, and string representations are intentionally unrestricted.(Part of a series addressing #524)