From 7f453b3512309f0fe70635d6dcdea56f5c99fc39 Mon Sep 17 00:00:00 2001 From: David Eustis Date: Thu, 7 May 2026 01:00:31 +0000 Subject: [PATCH 1/4] pybind: Add S1ChordAngle bindings 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. --- src/python/BUILD.bazel | 16 ++ src/python/module.cc | 4 + src/python/s1chord_angle_bindings.cc | 230 ++++++++++++++++++ src/python/s1chord_angle_test.py | 336 +++++++++++++++++++++++++++ 4 files changed, 586 insertions(+) create mode 100644 src/python/s1chord_angle_bindings.cc create mode 100644 src/python/s1chord_angle_test.py diff --git a/src/python/BUILD.bazel b/src/python/BUILD.bazel index a0d83669..86fc9aa4 100644 --- a/src/python/BUILD.bazel +++ b/src/python/BUILD.bazel @@ -30,6 +30,7 @@ pybind_extension( ":r2point_bindings", ":r2rect_bindings", ":s1angle_bindings", + ":s1chord_angle_bindings", ":s1interval_bindings", ":s2cell_id_bindings", ":s2latlng_bindings", @@ -69,6 +70,15 @@ pybind_library( ], ) +pybind_library( + name = "s1chord_angle_bindings", + srcs = ["s1chord_angle_bindings.cc"], + deps = [ + "//:s2", + "@abseil-cpp//absl/strings", + ], +) + pybind_library( name = "s1interval_bindings", srcs = ["s1interval_bindings.cc"], @@ -119,6 +129,12 @@ py_test( deps = [":s2geometry_pybind"], ) +py_test( + name = "s1chord_angle_test", + srcs = ["s1chord_angle_test.py"], + deps = [":s2geometry_pybind"], +) + py_test( name = "r2point_test", srcs = ["r2point_test.py"], diff --git a/src/python/module.cc b/src/python/module.cc index 91ef57ad..701899ca 100644 --- a/src/python/module.cc +++ b/src/python/module.cc @@ -8,6 +8,7 @@ void bind_r1interval(py::module& m); void bind_r2point(py::module& m); void bind_r2rect(py::module& m); void bind_s1angle(py::module& m); +void bind_s1chord_angle(py::module& m); void bind_s1interval(py::module& m); void bind_s2cell_id(py::module& m); void bind_s2latlng(py::module& m); @@ -33,6 +34,9 @@ PYBIND11_MODULE(s2geometry_bindings, m) { // Deps: s2point bind_s1angle(m); + // Deps: s1angle, s2point + bind_s1chord_angle(m); + // Deps: s1angle, s2point bind_s2latlng(m); diff --git a/src/python/s1chord_angle_bindings.cc b/src/python/s1chord_angle_bindings.cc new file mode 100644 index 00000000..3bfcaf22 --- /dev/null +++ b/src/python/s1chord_angle_bindings.cc @@ -0,0 +1,230 @@ +#include +#include + +#include +#include +#include + +#include "absl/strings/str_cat.h" +#include "s2/s1angle.h" +#include "s2/s1chord_angle.h" +#include "s2/s2point.h" + +namespace py = pybind11; + +namespace { + +void MaybeThrowNotUnitLength(const S2Point& p, const char* name) { + // Use a generous tolerance; this matches the spirit of the C++ + // ABSL_DCHECK(S2::IsUnitLength(x)) which uses ~4 * DBL_EPSILON. + double norm = p.Norm(); + if (std::abs(norm - 1.0) > 1e-12) { + throw py::value_error( + absl::StrCat(name, " must be a unit-length vector (norm=", norm, ")")); + } +} + +void MaybeThrowSpecialForArithmetic(const S1ChordAngle& a, const char* name) { + if (a.is_special()) { + throw py::value_error( + absl::StrCat(name, " must not be Negative() or Infinity() " + "for arithmetic")); + } +} + +void MaybeThrowNegativeLength2(double length2) { + if (length2 < 0) { + throw py::value_error( + absl::StrCat("length2 must be non-negative, got ", length2)); + } +} + +} // namespace + +void bind_s1chord_angle(py::module& m) { + py::class_(m, "S1ChordAngle", + "Represents the angle subtended by a chord on the unit sphere.\n\n" + "S1ChordAngle can represent angles between 0 and Pi radians. It is\n" + "more efficient than S1Angle for computing and comparing distances,\n" + "but loses some accuracy as the angle approaches Pi radians.\n\n" + "See s2/s1chord_angle.h for comprehensive documentation, including\n" + "accuracy analysis and guidance on when to prefer S1Angle.") + // Constructors + .def(py::init<>(), "Default constructor creates a zero chord angle") + .def(py::init([](const S2Point& x, const S2Point& y) { + MaybeThrowNotUnitLength(x, "x"); + MaybeThrowNotUnitLength(y, "y"); + return S1ChordAngle(x, y); + }), + py::arg("x"), py::arg("y"), + "Construct the chord angle between two unit-length points.\n\n" + "Raises ValueError if either point is not unit-length.") + .def(py::init(), py::arg("angle"), + "Construct from an S1Angle.\n\n" + "Angles outside [0, Pi] are mapped as follows:\n" + " Infinity() -> Infinity()\n" + " negative -> Negative()\n" + " > Pi -> Straight()\n" + "This conversion is relatively expensive; prefer to convert at\n" + "the boundaries of your algorithm.") + + // Factory methods + .def_static("from_radians", &S1ChordAngle::Radians, py::arg("radians"), + "Construct a chord angle from an angle in radians") + .def_static("from_degrees", &S1ChordAngle::Degrees, py::arg("degrees"), + "Construct a chord angle from an angle in degrees") + .def_static("from_e5", &S1ChordAngle::E5, py::arg("e5"), + "Construct a chord angle from the E5 representation") + .def_static("from_e6", &S1ChordAngle::E6, py::arg("e6"), + "Construct a chord angle from the E6 representation") + .def_static("from_e7", &S1ChordAngle::E7, py::arg("e7"), + "Construct a chord angle from the E7 representation") + .def_static("fast_upper_bound_from", &S1ChordAngle::FastUpperBoundFrom, + py::arg("angle"), + "Return a fast upper bound on the given S1Angle.\n\n" + "Accurate to within 1% for distances up to about 3100km on\n" + "the Earth's surface. Much faster than the S1Angle\n" + "constructor.") + .def_static("from_length2", [](double length2) { + MaybeThrowNegativeLength2(length2); + return S1ChordAngle::FromLength2(length2); + }, py::arg("length2"), + "Construct a chord angle from the squared chord length.\n\n" + "The argument is clamped to a maximum of 4.0 to handle roundoff.\n" + "Raises ValueError if the argument is negative.") + .def_static("zero", &S1ChordAngle::Zero, "Return the zero chord angle") + .def_static("right", &S1ChordAngle::Right, + "Return a 90-degree chord angle") + .def_static("straight", &S1ChordAngle::Straight, + "Return a 180-degree chord angle (the maximum finite value)") + .def_static("infinity", &S1ChordAngle::Infinity, + "Return a chord angle larger than any finite chord angle") + .def_static("negative", &S1ChordAngle::Negative, + "Return a chord angle smaller than Zero()") + + // Properties + .def_property_readonly("length2", &S1ChordAngle::length2, + "The squared chord length (most clients will " + "not need this)") + .def_property_readonly("radians", &S1ChordAngle::radians, + "The angle in radians.\n\n" + "Note: this performs a trigonometric conversion and should be\n" + "avoided in inner loops.") + .def_property_readonly("degrees", &S1ChordAngle::degrees, + "The angle in degrees.\n\n" + "Note: this performs a trigonometric conversion and should be\n" + "avoided in inner loops.") + .def_property_readonly("e5", &S1ChordAngle::e5, + "The E5 representation (degrees * 1e5, rounded)") + .def_property_readonly("e6", &S1ChordAngle::e6, + "The E6 representation (degrees * 1e6, rounded)") + .def_property_readonly("e7", &S1ChordAngle::e7, + "The E7 representation (degrees * 1e7, rounded)") + + // Predicates + .def("is_zero", &S1ChordAngle::is_zero, + "Return true if this is exactly zero") + .def("is_negative", &S1ChordAngle::is_negative, + "Return true if this is less than zero (e.g. Negative())") + .def("is_infinity", &S1ChordAngle::is_infinity, + "Return true if this is the Infinity() sentinel") + .def("is_special", &S1ChordAngle::is_special, + "Return true if this is Negative() or Infinity()") + .def("is_valid", &S1ChordAngle::is_valid, + "Return true if the internal representation is valid.\n\n" + "Negative() and Infinity() are both considered valid.") + + // Geometric operations + .def("to_angle", &S1ChordAngle::ToAngle, + "Convert to an S1Angle.\n\n" + "Infinity() converts to S1Angle::Infinity(); Negative() converts\n" + "to an unspecified negative S1Angle. Uses trigonometric functions\n" + "and should be avoided in inner loops.") + .def("sin", [](const S1ChordAngle& self) { return sin(self); }, + "Return the sine of the chord angle.\n\n" + "More accurate and efficient than converting to S1Angle first.") + .def("cos", [](const S1ChordAngle& self) { return cos(self); }, + "Return the cosine of the chord angle.\n\n" + "More accurate and efficient than converting to S1Angle first.") + .def("tan", [](const S1ChordAngle& self) { return tan(self); }, + "Return the tangent of the chord angle.\n\n" + "More accurate and efficient than converting to S1Angle first.") + .def("sin2", [](const S1ChordAngle& self) { return sin2(self); }, + "Return sin(self) squared, computed more efficiently than sin()**2") + .def("successor", &S1ChordAngle::Successor, + "Return the smallest representable S1ChordAngle larger than this.\n\n" + "Useful for converting a '<' comparison to a '<=' comparison.\n" + "Special cases:\n" + " Negative().successor() == Zero()\n" + " Straight().successor() == Infinity()\n" + " Infinity().successor() == Infinity()") + .def("predecessor", &S1ChordAngle::Predecessor, + "Return the largest representable S1ChordAngle smaller than this.\n\n" + "Special cases:\n" + " Infinity().predecessor() == Straight()\n" + " Zero().predecessor() == Negative()\n" + " Negative().predecessor() == Negative()") + .def("plus_error", &S1ChordAngle::PlusError, py::arg("error"), + "Return a chord angle adjusted by the given error bound.\n\n" + "The error may be positive or negative and should typically be\n" + "a value returned by one of the error-bound methods.") + .def("s2_point_constructor_max_error", + &S1ChordAngle::GetS2PointConstructorMaxError, + "Return the maximum error in length2 for the two-point constructor") + .def("s1_angle_constructor_max_error", + &S1ChordAngle::GetS1AngleConstructorMaxError, + "Return the maximum error in length2 for the S1Angle constructor") + + // Operators + .def(py::self == py::self, "Return true if chord angles are equal") + .def(py::self != py::self, "Return true if chord angles are not equal") + .def(py::self < py::self, + "Return true if this is less than other (by length2)") + .def(py::self > py::self, + "Return true if this is greater than other (by length2)") + .def(py::self <= py::self, + "Return true if this is less than or equal to other") + .def(py::self >= py::self, + "Return true if this is greater than or equal to other") + .def("__add__", [](const S1ChordAngle& a, const S1ChordAngle& b) { + MaybeThrowSpecialForArithmetic(a, "left operand"); + MaybeThrowSpecialForArithmetic(b, "right operand"); + return a + b; + }, py::is_operator(), + "Add two chord angles, clamping the result to [0, Pi].\n\n" + "Raises ValueError if either operand is Negative() or Infinity().") + .def("__sub__", [](const S1ChordAngle& a, const S1ChordAngle& b) { + MaybeThrowSpecialForArithmetic(a, "left operand"); + MaybeThrowSpecialForArithmetic(b, "right operand"); + return a - b; + }, 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) { + MaybeThrowSpecialForArithmetic(self, "left operand"); + MaybeThrowSpecialForArithmetic(other, "right operand"); + self += other; + return self; + }, py::is_operator(), "In-place addition") + .def("__isub__", [](S1ChordAngle& self, const S1ChordAngle& other) { + MaybeThrowSpecialForArithmetic(self, "left operand"); + MaybeThrowSpecialForArithmetic(other, "right operand"); + self -= other; + return self; + }, py::is_operator(), "In-place subtraction") + .def("__hash__", [](const S1ChordAngle& self) { + return std::hash()(self.length2()); + }) + + // String representation + .def("__repr__", [](const S1ChordAngle& a) { + std::ostringstream oss; + oss << "S1ChordAngle(" << a << ")"; + return oss.str(); + }) + .def("__str__", [](const S1ChordAngle& a) { + std::ostringstream oss; + oss << a; + return oss.str(); + }); +} diff --git a/src/python/s1chord_angle_test.py b/src/python/s1chord_angle_test.py new file mode 100644 index 00000000..4acfad9e --- /dev/null +++ b/src/python/s1chord_angle_test.py @@ -0,0 +1,336 @@ +"""Tests for S1ChordAngle pybind11 bindings.""" + +import math +import unittest +import s2geometry_pybind as s2 + + +class TestS1ChordAngle(unittest.TestCase): + """Test cases for S1ChordAngle bindings.""" + + # Constructors + + def test_default_constructor(self): + a = s2.S1ChordAngle() + self.assertTrue(a.is_zero()) + self.assertEqual(a.length2, 0.0) + + def test_constructor_from_two_points(self): + x = s2.S2Point(1.0, 0.0, 0.0) + y = s2.S2Point(0.0, 1.0, 0.0) + a = s2.S1ChordAngle(x, y) + # Chord length between orthogonal unit vectors is sqrt(2); + # length2 = 2. + self.assertAlmostEqual(a.length2, 2.0) + self.assertAlmostEqual(a.radians, math.pi / 2) + + def test_constructor_from_same_point(self): + p = s2.S2Point(1.0, 0.0, 0.0) + a = s2.S1ChordAngle(p, p) + self.assertTrue(a.is_zero()) + + def test_constructor_from_antipodal_points(self): + x = s2.S2Point(1.0, 0.0, 0.0) + y = s2.S2Point(-1.0, 0.0, 0.0) + a = s2.S1ChordAngle(x, y) + # Length2 for antipodal is 4. + self.assertAlmostEqual(a.length2, 4.0) + self.assertAlmostEqual(a.radians, math.pi) + + def test_constructor_non_unit_length_raises(self): + bad = s2.S2Point(2.0, 0.0, 0.0) + ok = s2.S2Point(1.0, 0.0, 0.0) + with self.assertRaises(ValueError): + s2.S1ChordAngle(bad, ok) + with self.assertRaises(ValueError): + s2.S1ChordAngle(ok, bad) + + def test_constructor_from_s1_angle(self): + angle = s2.S1Angle.from_degrees(90.0) + a = s2.S1ChordAngle(angle) + self.assertAlmostEqual(a.length2, 2.0) + + def test_constructor_from_s1_angle_negative(self): + # Negative angles map to Negative(). + a = s2.S1ChordAngle(s2.S1Angle.from_radians(-1.0)) + self.assertTrue(a.is_negative()) + + def test_constructor_from_s1_angle_infinity(self): + a = s2.S1ChordAngle(s2.S1Angle.infinity()) + self.assertTrue(a.is_infinity()) + + def test_constructor_from_s1_angle_larger_than_pi_clamps(self): + a = s2.S1ChordAngle(s2.S1Angle.from_radians(math.pi + 1.0)) + # Clamped to Straight() (length2 == 4). + self.assertAlmostEqual(a.length2, 4.0) + + # Factory methods + + def test_from_radians(self): + a = s2.S1ChordAngle.from_radians(math.pi / 2) + self.assertAlmostEqual(a.length2, 2.0) + + def test_from_degrees(self): + a = s2.S1ChordAngle.from_degrees(90.0) + self.assertAlmostEqual(a.length2, 2.0) + + def test_from_e5(self): + a = s2.S1ChordAngle.from_e5(9000000) + self.assertAlmostEqual(a.degrees, 90.0, places=3) + + def test_from_e6(self): + a = s2.S1ChordAngle.from_e6(90000000) + self.assertAlmostEqual(a.degrees, 90.0, places=5) + + def test_from_e7(self): + a = s2.S1ChordAngle.from_e7(900000000) + self.assertAlmostEqual(a.degrees, 90.0, places=7) + + def test_fast_upper_bound_from(self): + angle = s2.S1Angle.from_radians(0.1) + bound = s2.S1ChordAngle.fast_upper_bound_from(angle) + exact = s2.S1ChordAngle(angle) + # The fast bound is an upper bound on the exact value. + self.assertGreaterEqual(bound.length2, exact.length2) + + def test_from_length2(self): + a = s2.S1ChordAngle.from_length2(2.0) + self.assertAlmostEqual(a.length2, 2.0) + + def test_from_length2_clamps_at_4(self): + # Values above 4 are clamped (handles roundoff). + a = s2.S1ChordAngle.from_length2(5.0) + self.assertAlmostEqual(a.length2, 4.0) + + def test_from_length2_negative_raises(self): + with self.assertRaises(ValueError): + s2.S1ChordAngle.from_length2(-0.1) + + def test_zero(self): + a = s2.S1ChordAngle.zero() + self.assertTrue(a.is_zero()) + self.assertEqual(a.length2, 0.0) + + def test_right(self): + a = s2.S1ChordAngle.right() + self.assertAlmostEqual(a.length2, 2.0) + self.assertAlmostEqual(a.radians, math.pi / 2) + + def test_straight(self): + a = s2.S1ChordAngle.straight() + self.assertAlmostEqual(a.length2, 4.0) + self.assertAlmostEqual(a.radians, math.pi) + + def test_infinity(self): + a = s2.S1ChordAngle.infinity() + self.assertTrue(a.is_infinity()) + self.assertTrue(a.is_special()) + + def test_negative(self): + a = s2.S1ChordAngle.negative() + self.assertTrue(a.is_negative()) + self.assertTrue(a.is_special()) + + # Properties + + def test_length2(self): + a = s2.S1ChordAngle.from_length2(1.5) + self.assertAlmostEqual(a.length2, 1.5) + + def test_radians(self): + a = s2.S1ChordAngle.from_radians(1.0) + self.assertAlmostEqual(a.radians, 1.0) + + def test_degrees(self): + a = s2.S1ChordAngle.from_degrees(45.0) + self.assertAlmostEqual(a.degrees, 45.0) + + def test_e5_e6_e7(self): + a = s2.S1ChordAngle.from_degrees(45.0) + self.assertEqual(a.e5, 4500000) + self.assertEqual(a.e6, 45000000) + self.assertEqual(a.e7, 450000000) + + # Predicates + + def test_is_zero(self): + self.assertTrue(s2.S1ChordAngle.zero().is_zero()) + self.assertFalse(s2.S1ChordAngle.right().is_zero()) + + def test_is_negative(self): + self.assertTrue(s2.S1ChordAngle.negative().is_negative()) + self.assertFalse(s2.S1ChordAngle.zero().is_negative()) + + def test_is_infinity(self): + self.assertTrue(s2.S1ChordAngle.infinity().is_infinity()) + self.assertFalse(s2.S1ChordAngle.straight().is_infinity()) + + def test_is_special(self): + self.assertTrue(s2.S1ChordAngle.negative().is_special()) + self.assertTrue(s2.S1ChordAngle.infinity().is_special()) + self.assertFalse(s2.S1ChordAngle.zero().is_special()) + self.assertFalse(s2.S1ChordAngle.straight().is_special()) + + def test_is_valid(self): + self.assertTrue(s2.S1ChordAngle.zero().is_valid()) + self.assertTrue(s2.S1ChordAngle.straight().is_valid()) + self.assertTrue(s2.S1ChordAngle.negative().is_valid()) + self.assertTrue(s2.S1ChordAngle.infinity().is_valid()) + + # Geometric operations + + def test_to_angle(self): + a = s2.S1ChordAngle.from_degrees(90.0) + angle = a.to_angle() + self.assertAlmostEqual(angle.degrees, 90.0) + + def test_sin_cos_tan(self): + a = s2.S1ChordAngle.from_degrees(30.0) + self.assertAlmostEqual(a.sin(), 0.5, places=10) + self.assertAlmostEqual(a.cos(), math.sqrt(3) / 2, places=10) + self.assertAlmostEqual(a.tan(), 1.0 / math.sqrt(3), places=10) + + def test_sin2(self): + a = s2.S1ChordAngle.from_degrees(30.0) + self.assertAlmostEqual(a.sin2(), 0.25, places=10) + + def test_successor(self): + # Negative().successor() == Zero(). + self.assertEqual( + s2.S1ChordAngle.negative().successor(), s2.S1ChordAngle.zero()) + # Straight().successor() == Infinity(). + self.assertEqual( + s2.S1ChordAngle.straight().successor(), s2.S1ChordAngle.infinity()) + # Infinity().successor() == Infinity() (fixed point). + self.assertEqual( + s2.S1ChordAngle.infinity().successor(), s2.S1ChordAngle.infinity()) + # Successor is strictly greater for non-special values. + a = s2.S1ChordAngle.from_degrees(45.0) + self.assertGreater(a.successor(), a) + + def test_predecessor(self): + # Infinity().predecessor() == Straight(). + self.assertEqual( + s2.S1ChordAngle.infinity().predecessor(), + s2.S1ChordAngle.straight()) + # Zero().predecessor() == Negative(). + self.assertEqual( + s2.S1ChordAngle.zero().predecessor(), s2.S1ChordAngle.negative()) + # Negative().predecessor() == Negative() (fixed point). + self.assertEqual( + s2.S1ChordAngle.negative().predecessor(), + s2.S1ChordAngle.negative()) + a = s2.S1ChordAngle.from_degrees(45.0) + self.assertLess(a.predecessor(), a) + + def test_plus_error(self): + a = s2.S1ChordAngle.from_degrees(45.0) + adjusted = a.plus_error(0.0) + self.assertAlmostEqual(a.length2, adjusted.length2) + adjusted2 = a.plus_error(1e-10) + self.assertGreater(adjusted2.length2, a.length2) + + def test_constructor_max_error(self): + a = s2.S1ChordAngle.from_degrees(45.0) + self.assertGreater(a.s2_point_constructor_max_error(), 0.0) + self.assertGreater(a.s1_angle_constructor_max_error(), 0.0) + + # Operators + + def test_equality(self): + a = s2.S1ChordAngle.from_degrees(45.0) + b = s2.S1ChordAngle.from_degrees(45.0) + c = s2.S1ChordAngle.from_degrees(90.0) + self.assertTrue(a == b) + self.assertTrue(a != c) + + def test_comparison(self): + a = s2.S1ChordAngle.from_degrees(30.0) + b = s2.S1ChordAngle.from_degrees(60.0) + self.assertTrue(a < b) + self.assertTrue(b > a) + self.assertTrue(a <= a) + self.assertTrue(a >= a) + + def test_comparison_special_values(self): + # Negative < Zero < finite < Infinity. + neg = s2.S1ChordAngle.negative() + zero = s2.S1ChordAngle.zero() + finite = s2.S1ChordAngle.from_degrees(45.0) + inf = s2.S1ChordAngle.infinity() + self.assertLess(neg, zero) + self.assertLess(zero, finite) + self.assertLess(finite, inf) + + def test_add(self): + a = s2.S1ChordAngle.from_degrees(30.0) + b = s2.S1ChordAngle.from_degrees(60.0) + total = a + b + self.assertAlmostEqual(total.degrees, 90.0, places=5) + + def test_add_clamps_to_pi(self): + # Adding two large chord angles clamps the result to Straight(). + a = s2.S1ChordAngle.from_degrees(120.0) + b = s2.S1ChordAngle.from_degrees(120.0) + total = a + b + self.assertAlmostEqual(total.length2, 4.0) + + def test_add_special_raises(self): + a = s2.S1ChordAngle.from_degrees(30.0) + with self.assertRaises(ValueError): + _ = a + s2.S1ChordAngle.negative() + with self.assertRaises(ValueError): + _ = s2.S1ChordAngle.infinity() + a + + def test_sub(self): + a = s2.S1ChordAngle.from_degrees(60.0) + b = s2.S1ChordAngle.from_degrees(30.0) + diff = a - b + self.assertAlmostEqual(diff.degrees, 30.0, places=5) + + def test_sub_clamps_at_zero(self): + # Subtracting a larger chord angle clamps to zero. + a = s2.S1ChordAngle.from_degrees(30.0) + b = s2.S1ChordAngle.from_degrees(60.0) + diff = a - b + self.assertAlmostEqual(diff.length2, 0.0) + + def test_sub_special_raises(self): + a = s2.S1ChordAngle.from_degrees(30.0) + with self.assertRaises(ValueError): + _ = a - s2.S1ChordAngle.negative() + + def test_iadd(self): + a = s2.S1ChordAngle.from_degrees(30.0) + a += s2.S1ChordAngle.from_degrees(60.0) + self.assertAlmostEqual(a.degrees, 90.0, places=5) + + def test_isub(self): + a = s2.S1ChordAngle.from_degrees(60.0) + a -= s2.S1ChordAngle.from_degrees(30.0) + self.assertAlmostEqual(a.degrees, 30.0, places=5) + + def test_hash(self): + a = s2.S1ChordAngle.from_degrees(45.0) + b = s2.S1ChordAngle.from_degrees(45.0) + self.assertEqual(hash(a), hash(b)) + s = {a, b} + self.assertEqual(len(s), 1) + + # String representation + + def test_repr(self): + a = s2.S1ChordAngle.from_degrees(90.0) + r = repr(a) + self.assertTrue(r.startswith("S1ChordAngle(")) + self.assertTrue(r.endswith(")")) + + def test_str(self): + a = s2.S1ChordAngle.from_degrees(90.0) + # The stream inserter prints the equivalent S1Angle. + s = str(a) + self.assertIn("90", s) + + +if __name__ == "__main__": + unittest.main() From 0d07e4f7e3a89c3ac9305470c3fd7cc80a413c8e Mon Sep 17 00:00:00 2001 From: David Eustis Date: Wed, 13 May 2026 04:11:47 +0000 Subject: [PATCH 2/4] pybind: Address review comments on S1ChordAngle 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 --- src/python/s1chord_angle_bindings.cc | 58 +-------------- src/python/s1chord_angle_test.py | 107 ++------------------------- 2 files changed, 10 insertions(+), 155 deletions(-) diff --git a/src/python/s1chord_angle_bindings.cc b/src/python/s1chord_angle_bindings.cc index 3bfcaf22..db1baa0b 100644 --- a/src/python/s1chord_angle_bindings.cc +++ b/src/python/s1chord_angle_bindings.cc @@ -2,25 +2,23 @@ #include #include -#include #include #include "absl/strings/str_cat.h" #include "s2/s1angle.h" #include "s2/s1chord_angle.h" #include "s2/s2point.h" +#include "s2/s2pointutil.h" namespace py = pybind11; namespace { void MaybeThrowNotUnitLength(const S2Point& p, const char* name) { - // Use a generous tolerance; this matches the spirit of the C++ - // ABSL_DCHECK(S2::IsUnitLength(x)) which uses ~4 * DBL_EPSILON. - double norm = p.Norm(); - if (std::abs(norm - 1.0) > 1e-12) { + if (!S2::IsUnitLength(p)) { throw py::value_error( - absl::StrCat(name, " must be a unit-length vector (norm=", norm, ")")); + absl::StrCat(name, " must be a unit-length vector (norm=", + p.Norm(), ")")); } } @@ -32,13 +30,6 @@ void MaybeThrowSpecialForArithmetic(const S1ChordAngle& a, const char* name) { } } -void MaybeThrowNegativeLength2(double length2) { - if (length2 < 0) { - throw py::value_error( - absl::StrCat("length2 must be non-negative, got ", length2)); - } -} - } // namespace void bind_s1chord_angle(py::module& m) { @@ -79,19 +70,6 @@ void bind_s1chord_angle(py::module& m) { "Construct a chord angle from the E6 representation") .def_static("from_e7", &S1ChordAngle::E7, py::arg("e7"), "Construct a chord angle from the E7 representation") - .def_static("fast_upper_bound_from", &S1ChordAngle::FastUpperBoundFrom, - py::arg("angle"), - "Return a fast upper bound on the given S1Angle.\n\n" - "Accurate to within 1% for distances up to about 3100km on\n" - "the Earth's surface. Much faster than the S1Angle\n" - "constructor.") - .def_static("from_length2", [](double length2) { - MaybeThrowNegativeLength2(length2); - return S1ChordAngle::FromLength2(length2); - }, py::arg("length2"), - "Construct a chord angle from the squared chord length.\n\n" - "The argument is clamped to a maximum of 4.0 to handle roundoff.\n" - "Raises ValueError if the argument is negative.") .def_static("zero", &S1ChordAngle::Zero, "Return the zero chord angle") .def_static("right", &S1ChordAngle::Right, "Return a 90-degree chord angle") @@ -103,9 +81,6 @@ void bind_s1chord_angle(py::module& m) { "Return a chord angle smaller than Zero()") // Properties - .def_property_readonly("length2", &S1ChordAngle::length2, - "The squared chord length (most clients will " - "not need this)") .def_property_readonly("radians", &S1ChordAngle::radians, "The angle in radians.\n\n" "Note: this performs a trigonometric conversion and should be\n" @@ -149,31 +124,6 @@ void bind_s1chord_angle(py::module& m) { .def("tan", [](const S1ChordAngle& self) { return tan(self); }, "Return the tangent of the chord angle.\n\n" "More accurate and efficient than converting to S1Angle first.") - .def("sin2", [](const S1ChordAngle& self) { return sin2(self); }, - "Return sin(self) squared, computed more efficiently than sin()**2") - .def("successor", &S1ChordAngle::Successor, - "Return the smallest representable S1ChordAngle larger than this.\n\n" - "Useful for converting a '<' comparison to a '<=' comparison.\n" - "Special cases:\n" - " Negative().successor() == Zero()\n" - " Straight().successor() == Infinity()\n" - " Infinity().successor() == Infinity()") - .def("predecessor", &S1ChordAngle::Predecessor, - "Return the largest representable S1ChordAngle smaller than this.\n\n" - "Special cases:\n" - " Infinity().predecessor() == Straight()\n" - " Zero().predecessor() == Negative()\n" - " Negative().predecessor() == Negative()") - .def("plus_error", &S1ChordAngle::PlusError, py::arg("error"), - "Return a chord angle adjusted by the given error bound.\n\n" - "The error may be positive or negative and should typically be\n" - "a value returned by one of the error-bound methods.") - .def("s2_point_constructor_max_error", - &S1ChordAngle::GetS2PointConstructorMaxError, - "Return the maximum error in length2 for the two-point constructor") - .def("s1_angle_constructor_max_error", - &S1ChordAngle::GetS1AngleConstructorMaxError, - "Return the maximum error in length2 for the S1Angle constructor") // Operators .def(py::self == py::self, "Return true if chord angles are equal") diff --git a/src/python/s1chord_angle_test.py b/src/python/s1chord_angle_test.py index 4acfad9e..ef782d5c 100644 --- a/src/python/s1chord_angle_test.py +++ b/src/python/s1chord_angle_test.py @@ -13,15 +13,11 @@ class TestS1ChordAngle(unittest.TestCase): def test_default_constructor(self): a = s2.S1ChordAngle() self.assertTrue(a.is_zero()) - self.assertEqual(a.length2, 0.0) def test_constructor_from_two_points(self): x = s2.S2Point(1.0, 0.0, 0.0) y = s2.S2Point(0.0, 1.0, 0.0) a = s2.S1ChordAngle(x, y) - # Chord length between orthogonal unit vectors is sqrt(2); - # length2 = 2. - self.assertAlmostEqual(a.length2, 2.0) self.assertAlmostEqual(a.radians, math.pi / 2) def test_constructor_from_same_point(self): @@ -33,8 +29,6 @@ def test_constructor_from_antipodal_points(self): x = s2.S2Point(1.0, 0.0, 0.0) y = s2.S2Point(-1.0, 0.0, 0.0) a = s2.S1ChordAngle(x, y) - # Length2 for antipodal is 4. - self.assertAlmostEqual(a.length2, 4.0) self.assertAlmostEqual(a.radians, math.pi) def test_constructor_non_unit_length_raises(self): @@ -48,10 +42,9 @@ def test_constructor_non_unit_length_raises(self): def test_constructor_from_s1_angle(self): angle = s2.S1Angle.from_degrees(90.0) a = s2.S1ChordAngle(angle) - self.assertAlmostEqual(a.length2, 2.0) + self.assertAlmostEqual(a.radians, math.pi / 2) def test_constructor_from_s1_angle_negative(self): - # Negative angles map to Negative(). a = s2.S1ChordAngle(s2.S1Angle.from_radians(-1.0)) self.assertTrue(a.is_negative()) @@ -61,64 +54,28 @@ def test_constructor_from_s1_angle_infinity(self): def test_constructor_from_s1_angle_larger_than_pi_clamps(self): a = s2.S1ChordAngle(s2.S1Angle.from_radians(math.pi + 1.0)) - # Clamped to Straight() (length2 == 4). - self.assertAlmostEqual(a.length2, 4.0) + self.assertEqual(a, s2.S1ChordAngle.straight()) # Factory methods def test_from_radians(self): a = s2.S1ChordAngle.from_radians(math.pi / 2) - self.assertAlmostEqual(a.length2, 2.0) + self.assertAlmostEqual(a.radians, math.pi / 2) def test_from_degrees(self): a = s2.S1ChordAngle.from_degrees(90.0) - self.assertAlmostEqual(a.length2, 2.0) - - def test_from_e5(self): - a = s2.S1ChordAngle.from_e5(9000000) - self.assertAlmostEqual(a.degrees, 90.0, places=3) - - def test_from_e6(self): - a = s2.S1ChordAngle.from_e6(90000000) - self.assertAlmostEqual(a.degrees, 90.0, places=5) - - def test_from_e7(self): - a = s2.S1ChordAngle.from_e7(900000000) - self.assertAlmostEqual(a.degrees, 90.0, places=7) - - def test_fast_upper_bound_from(self): - angle = s2.S1Angle.from_radians(0.1) - bound = s2.S1ChordAngle.fast_upper_bound_from(angle) - exact = s2.S1ChordAngle(angle) - # The fast bound is an upper bound on the exact value. - self.assertGreaterEqual(bound.length2, exact.length2) - - def test_from_length2(self): - a = s2.S1ChordAngle.from_length2(2.0) - self.assertAlmostEqual(a.length2, 2.0) - - def test_from_length2_clamps_at_4(self): - # Values above 4 are clamped (handles roundoff). - a = s2.S1ChordAngle.from_length2(5.0) - self.assertAlmostEqual(a.length2, 4.0) - - def test_from_length2_negative_raises(self): - with self.assertRaises(ValueError): - s2.S1ChordAngle.from_length2(-0.1) + self.assertAlmostEqual(a.degrees, 90.0) def test_zero(self): a = s2.S1ChordAngle.zero() self.assertTrue(a.is_zero()) - self.assertEqual(a.length2, 0.0) def test_right(self): a = s2.S1ChordAngle.right() - self.assertAlmostEqual(a.length2, 2.0) self.assertAlmostEqual(a.radians, math.pi / 2) def test_straight(self): a = s2.S1ChordAngle.straight() - self.assertAlmostEqual(a.length2, 4.0) self.assertAlmostEqual(a.radians, math.pi) def test_infinity(self): @@ -133,10 +90,6 @@ def test_negative(self): # Properties - def test_length2(self): - a = s2.S1ChordAngle.from_length2(1.5) - self.assertAlmostEqual(a.length2, 1.5) - def test_radians(self): a = s2.S1ChordAngle.from_radians(1.0) self.assertAlmostEqual(a.radians, 1.0) @@ -190,51 +143,6 @@ def test_sin_cos_tan(self): self.assertAlmostEqual(a.cos(), math.sqrt(3) / 2, places=10) self.assertAlmostEqual(a.tan(), 1.0 / math.sqrt(3), places=10) - def test_sin2(self): - a = s2.S1ChordAngle.from_degrees(30.0) - self.assertAlmostEqual(a.sin2(), 0.25, places=10) - - def test_successor(self): - # Negative().successor() == Zero(). - self.assertEqual( - s2.S1ChordAngle.negative().successor(), s2.S1ChordAngle.zero()) - # Straight().successor() == Infinity(). - self.assertEqual( - s2.S1ChordAngle.straight().successor(), s2.S1ChordAngle.infinity()) - # Infinity().successor() == Infinity() (fixed point). - self.assertEqual( - s2.S1ChordAngle.infinity().successor(), s2.S1ChordAngle.infinity()) - # Successor is strictly greater for non-special values. - a = s2.S1ChordAngle.from_degrees(45.0) - self.assertGreater(a.successor(), a) - - def test_predecessor(self): - # Infinity().predecessor() == Straight(). - self.assertEqual( - s2.S1ChordAngle.infinity().predecessor(), - s2.S1ChordAngle.straight()) - # Zero().predecessor() == Negative(). - self.assertEqual( - s2.S1ChordAngle.zero().predecessor(), s2.S1ChordAngle.negative()) - # Negative().predecessor() == Negative() (fixed point). - self.assertEqual( - s2.S1ChordAngle.negative().predecessor(), - s2.S1ChordAngle.negative()) - a = s2.S1ChordAngle.from_degrees(45.0) - self.assertLess(a.predecessor(), a) - - def test_plus_error(self): - a = s2.S1ChordAngle.from_degrees(45.0) - adjusted = a.plus_error(0.0) - self.assertAlmostEqual(a.length2, adjusted.length2) - adjusted2 = a.plus_error(1e-10) - self.assertGreater(adjusted2.length2, a.length2) - - def test_constructor_max_error(self): - a = s2.S1ChordAngle.from_degrees(45.0) - self.assertGreater(a.s2_point_constructor_max_error(), 0.0) - self.assertGreater(a.s1_angle_constructor_max_error(), 0.0) - # Operators def test_equality(self): @@ -269,11 +177,10 @@ def test_add(self): self.assertAlmostEqual(total.degrees, 90.0, places=5) def test_add_clamps_to_pi(self): - # Adding two large chord angles clamps the result to Straight(). a = s2.S1ChordAngle.from_degrees(120.0) b = s2.S1ChordAngle.from_degrees(120.0) total = a + b - self.assertAlmostEqual(total.length2, 4.0) + self.assertEqual(total, s2.S1ChordAngle.straight()) def test_add_special_raises(self): a = s2.S1ChordAngle.from_degrees(30.0) @@ -289,11 +196,10 @@ def test_sub(self): self.assertAlmostEqual(diff.degrees, 30.0, places=5) def test_sub_clamps_at_zero(self): - # Subtracting a larger chord angle clamps to zero. a = s2.S1ChordAngle.from_degrees(30.0) b = s2.S1ChordAngle.from_degrees(60.0) diff = a - b - self.assertAlmostEqual(diff.length2, 0.0) + self.assertTrue(diff.is_zero()) def test_sub_special_raises(self): a = s2.S1ChordAngle.from_degrees(30.0) @@ -327,7 +233,6 @@ def test_repr(self): def test_str(self): a = s2.S1ChordAngle.from_degrees(90.0) - # The stream inserter prints the equivalent S1Angle. s = str(a) self.assertIn("90", s) From f9ab8d62d20c66d514c03d6b66e3406df0b6cdf2 Mon Sep 17 00:00:00 2001 From: David Eustis Date: Wed, 13 May 2026 04:29:28 +0000 Subject: [PATCH 3/4] pybind: Fix edge cases and improve S1ChordAngle bindings quality - 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 to absl::Hash; 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 --- src/python/BUILD.bazel | 1 + src/python/s1chord_angle_bindings.cc | 70 ++++++++++++++++++---------- src/python/s1chord_angle_test.py | 47 ++++++++++++++++--- 3 files changed, 88 insertions(+), 30 deletions(-) diff --git a/src/python/BUILD.bazel b/src/python/BUILD.bazel index 86fc9aa4..0c9621cc 100644 --- a/src/python/BUILD.bazel +++ b/src/python/BUILD.bazel @@ -75,6 +75,7 @@ pybind_library( srcs = ["s1chord_angle_bindings.cc"], deps = [ "//:s2", + "@abseil-cpp//absl/hash", "@abseil-cpp//absl/strings", ], ) diff --git a/src/python/s1chord_angle_bindings.cc b/src/python/s1chord_angle_bindings.cc index db1baa0b..cd4a6cdd 100644 --- a/src/python/s1chord_angle_bindings.cc +++ b/src/python/s1chord_angle_bindings.cc @@ -4,6 +4,7 @@ #include #include +#include "absl/hash/hash.h" #include "absl/strings/str_cat.h" #include "s2/s1angle.h" #include "s2/s1chord_angle.h" @@ -22,11 +23,11 @@ void MaybeThrowNotUnitLength(const S2Point& p, const char* name) { } } -void MaybeThrowSpecialForArithmetic(const S1ChordAngle& a, const char* name) { +void MaybeThrowIfSpecial(const S1ChordAngle& a, const char* name) { if (a.is_special()) { throw py::value_error( - absl::StrCat(name, " must not be Negative() or Infinity() " - "for arithmetic")); + absl::StrCat(name, " must not be a special value " + "(negative() or infinity())")); } } @@ -89,12 +90,21 @@ void bind_s1chord_angle(py::module& m) { "The angle in degrees.\n\n" "Note: this performs a trigonometric conversion and should be\n" "avoided in inner loops.") - .def_property_readonly("e5", &S1ChordAngle::e5, - "The E5 representation (degrees * 1e5, rounded)") - .def_property_readonly("e6", &S1ChordAngle::e6, - "The E6 representation (degrees * 1e6, rounded)") - .def_property_readonly("e7", &S1ChordAngle::e7, - "The E7 representation (degrees * 1e7, rounded)") + .def_property_readonly("e5", [](const S1ChordAngle& self) { + MaybeThrowIfSpecial(self, "self"); + return self.e5(); + }, "The E5 representation (degrees * 1e5, rounded).\n\n" + "Raises ValueError if the angle is negative() or infinity().") + .def_property_readonly("e6", [](const S1ChordAngle& self) { + MaybeThrowIfSpecial(self, "self"); + return self.e6(); + }, "The E6 representation (degrees * 1e6, rounded).\n\n" + "Raises ValueError if the angle is negative() or infinity().") + .def_property_readonly("e7", [](const S1ChordAngle& self) { + MaybeThrowIfSpecial(self, "self"); + return self.e7(); + }, "The E7 representation (degrees * 1e7, rounded).\n\n" + "Raises ValueError if the angle is negative() or infinity().") // Predicates .def("is_zero", &S1ChordAngle::is_zero, @@ -115,15 +125,27 @@ void bind_s1chord_angle(py::module& m) { "Infinity() converts to S1Angle::Infinity(); Negative() converts\n" "to an unspecified negative S1Angle. Uses trigonometric functions\n" "and should be avoided in inner loops.") - .def("sin", [](const S1ChordAngle& self) { return sin(self); }, + .def("sin", [](const S1ChordAngle& self) { + MaybeThrowIfSpecial(self, "self"); + return sin(self); + }, "Return the sine of the chord angle.\n\n" - "More accurate and efficient than converting to S1Angle first.") - .def("cos", [](const S1ChordAngle& self) { return cos(self); }, + "More accurate and efficient than converting to S1Angle first.\n" + "Raises ValueError if the angle is negative() or infinity().") + .def("cos", [](const S1ChordAngle& self) { + MaybeThrowIfSpecial(self, "self"); + return cos(self); + }, "Return the cosine of the chord angle.\n\n" - "More accurate and efficient than converting to S1Angle first.") - .def("tan", [](const S1ChordAngle& self) { return tan(self); }, + "More accurate and efficient than converting to S1Angle first.\n" + "Raises ValueError if the angle is negative() or infinity().") + .def("tan", [](const S1ChordAngle& self) { + MaybeThrowIfSpecial(self, "self"); + return tan(self); + }, "Return the tangent of the chord angle.\n\n" - "More accurate and efficient than converting to S1Angle first.") + "More accurate and efficient than converting to S1Angle first.\n" + "Raises ValueError if the angle is negative() or infinity().") // Operators .def(py::self == py::self, "Return true if chord angles are equal") @@ -137,33 +159,33 @@ void bind_s1chord_angle(py::module& m) { .def(py::self >= py::self, "Return true if this is greater than or equal to other") .def("__add__", [](const S1ChordAngle& a, const S1ChordAngle& b) { - MaybeThrowSpecialForArithmetic(a, "left operand"); - MaybeThrowSpecialForArithmetic(b, "right operand"); + MaybeThrowIfSpecial(a, "left operand"); + MaybeThrowIfSpecial(b, "right operand"); return a + b; }, py::is_operator(), "Add two chord angles, clamping the result to [0, Pi].\n\n" "Raises ValueError if either operand is Negative() or Infinity().") .def("__sub__", [](const S1ChordAngle& a, const S1ChordAngle& b) { - MaybeThrowSpecialForArithmetic(a, "left operand"); - MaybeThrowSpecialForArithmetic(b, "right operand"); + MaybeThrowIfSpecial(a, "left operand"); + MaybeThrowIfSpecial(b, "right operand"); return a - b; }, 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) { - MaybeThrowSpecialForArithmetic(self, "left operand"); - MaybeThrowSpecialForArithmetic(other, "right operand"); + MaybeThrowIfSpecial(self, "left operand"); + MaybeThrowIfSpecial(other, "right operand"); self += other; return self; }, py::is_operator(), "In-place addition") .def("__isub__", [](S1ChordAngle& self, const S1ChordAngle& other) { - MaybeThrowSpecialForArithmetic(self, "left operand"); - MaybeThrowSpecialForArithmetic(other, "right operand"); + MaybeThrowIfSpecial(self, "left operand"); + MaybeThrowIfSpecial(other, "right operand"); self -= other; return self; }, py::is_operator(), "In-place subtraction") .def("__hash__", [](const S1ChordAngle& self) { - return std::hash()(self.length2()); + return absl::Hash()(self.length2()); }) // String representation diff --git a/src/python/s1chord_angle_test.py b/src/python/s1chord_angle_test.py index ef782d5c..79ed2fca 100644 --- a/src/python/s1chord_angle_test.py +++ b/src/python/s1chord_angle_test.py @@ -29,7 +29,7 @@ def test_constructor_from_antipodal_points(self): x = s2.S2Point(1.0, 0.0, 0.0) y = s2.S2Point(-1.0, 0.0, 0.0) a = s2.S1ChordAngle(x, y) - self.assertAlmostEqual(a.radians, math.pi) + self.assertEqual(a, s2.S1ChordAngle.straight()) def test_constructor_non_unit_length_raises(self): bad = s2.S2Point(2.0, 0.0, 0.0) @@ -104,6 +104,20 @@ def test_e5_e6_e7(self): self.assertEqual(a.e6, 45000000) self.assertEqual(a.e7, 450000000) + def test_e5_e6_e7_special_raises(self): + with self.assertRaises(ValueError): + _ = s2.S1ChordAngle.negative().e5 + with self.assertRaises(ValueError): + _ = s2.S1ChordAngle.infinity().e5 + with self.assertRaises(ValueError): + _ = s2.S1ChordAngle.negative().e6 + with self.assertRaises(ValueError): + _ = s2.S1ChordAngle.infinity().e6 + with self.assertRaises(ValueError): + _ = s2.S1ChordAngle.negative().e7 + with self.assertRaises(ValueError): + _ = s2.S1ChordAngle.infinity().e7 + # Predicates def test_is_zero(self): @@ -137,12 +151,31 @@ def test_to_angle(self): angle = a.to_angle() self.assertAlmostEqual(angle.degrees, 90.0) + def test_to_angle_special_values(self): + self.assertEqual(s2.S1ChordAngle.infinity().to_angle(), + s2.S1Angle.infinity()) + self.assertTrue(s2.S1ChordAngle.negative().to_angle().radians < 0) + def test_sin_cos_tan(self): a = s2.S1ChordAngle.from_degrees(30.0) self.assertAlmostEqual(a.sin(), 0.5, places=10) self.assertAlmostEqual(a.cos(), math.sqrt(3) / 2, places=10) self.assertAlmostEqual(a.tan(), 1.0 / math.sqrt(3), places=10) + def test_sin_cos_tan_special_raises(self): + with self.assertRaises(ValueError): + s2.S1ChordAngle.negative().sin() + with self.assertRaises(ValueError): + s2.S1ChordAngle.infinity().sin() + with self.assertRaises(ValueError): + s2.S1ChordAngle.negative().cos() + with self.assertRaises(ValueError): + s2.S1ChordAngle.infinity().cos() + with self.assertRaises(ValueError): + s2.S1ChordAngle.negative().tan() + with self.assertRaises(ValueError): + s2.S1ChordAngle.infinity().tan() + # Operators def test_equality(self): @@ -227,14 +260,16 @@ def test_hash(self): def test_repr(self): a = s2.S1ChordAngle.from_degrees(90.0) - r = repr(a) - self.assertTrue(r.startswith("S1ChordAngle(")) - self.assertTrue(r.endswith(")")) + self.assertEqual(repr(a), "S1ChordAngle(90.0000000)") def test_str(self): a = s2.S1ChordAngle.from_degrees(90.0) - s = str(a) - self.assertIn("90", s) + self.assertEqual(str(a), "90.0000000") + + def test_str_negative_sentinel(self): + # negative() is a sentinel value; its string output is implementation- + # defined but should represent a negative angle. + self.assertIn("-", str(s2.S1ChordAngle.negative())) if __name__ == "__main__": From 28626641ca8b7750c505be4e33d9e3aa50e5c5f0 Mon Sep 17 00:00:00 2001 From: David Eustis Date: Wed, 20 May 2026 22:50:45 +0000 Subject: [PATCH 4/4] pybind: Remove __iadd__/__isub__ from S1ChordAngle 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__. --- src/python/s1chord_angle_bindings.cc | 12 ------------ src/python/s1chord_angle_test.py | 4 ++-- 2 files changed, 2 insertions(+), 14 deletions(-) diff --git a/src/python/s1chord_angle_bindings.cc b/src/python/s1chord_angle_bindings.cc index cd4a6cdd..9710fce9 100644 --- a/src/python/s1chord_angle_bindings.cc +++ b/src/python/s1chord_angle_bindings.cc @@ -172,18 +172,6 @@ void bind_s1chord_angle(py::module& m) { }, 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) { - MaybeThrowIfSpecial(self, "left operand"); - MaybeThrowIfSpecial(other, "right operand"); - self += other; - return self; - }, py::is_operator(), "In-place addition") - .def("__isub__", [](S1ChordAngle& self, const S1ChordAngle& other) { - MaybeThrowIfSpecial(self, "left operand"); - MaybeThrowIfSpecial(other, "right operand"); - self -= other; - return self; - }, py::is_operator(), "In-place subtraction") .def("__hash__", [](const S1ChordAngle& self) { return absl::Hash()(self.length2()); }) diff --git a/src/python/s1chord_angle_test.py b/src/python/s1chord_angle_test.py index 79ed2fca..1183101a 100644 --- a/src/python/s1chord_angle_test.py +++ b/src/python/s1chord_angle_test.py @@ -239,12 +239,12 @@ def test_sub_special_raises(self): with self.assertRaises(ValueError): _ = a - s2.S1ChordAngle.negative() - def test_iadd(self): + def test_add_assign(self): a = s2.S1ChordAngle.from_degrees(30.0) a += s2.S1ChordAngle.from_degrees(60.0) self.assertAlmostEqual(a.degrees, 90.0, places=5) - def test_isub(self): + def test_sub_assign(self): a = s2.S1ChordAngle.from_degrees(60.0) a -= s2.S1ChordAngle.from_degrees(30.0) self.assertAlmostEqual(a.degrees, 30.0, places=5)