From b836aed10b76ae8f3017524ff5eb89427a3e7100 Mon Sep 17 00:00:00 2001 From: David Eustis Date: Tue, 7 Apr 2026 18:24:31 +0000 Subject: [PATCH 1/3] pybind: Add S1Angle bindings --- src/python/BUILD.bazel | 15 ++ src/python/module.cc | 2 + src/python/s1angle_bindings.cc | 158 ++++++++++++++++++ src/python/s1angle_test.py | 291 +++++++++++++++++++++++++++++++++ src/s2/s1angle.h | 10 +- 5 files changed, 475 insertions(+), 1 deletion(-) create mode 100644 src/python/s1angle_bindings.cc create mode 100644 src/python/s1angle_test.py diff --git a/src/python/BUILD.bazel b/src/python/BUILD.bazel index 3d9f83d3..11cdc98b 100644 --- a/src/python/BUILD.bazel +++ b/src/python/BUILD.bazel @@ -29,6 +29,7 @@ pybind_extension( ":r1interval_bindings", ":r2point_bindings", ":r2rect_bindings", + ":s1angle_bindings", ":s1interval_bindings", ":s2point_bindings", ], @@ -58,6 +59,14 @@ pybind_library( ], ) +pybind_library( + name = "s1angle_bindings", + srcs = ["s1angle_bindings.cc"], + deps = [ + "//:s2", + ], +) + pybind_library( name = "s1interval_bindings", srcs = ["s1interval_bindings.cc"], @@ -84,6 +93,12 @@ py_test( deps = [":s2geometry_pybind"], ) +py_test( + name = "s1angle_test", + srcs = ["s1angle_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 7652603a..40256ed5 100644 --- a/src/python/module.cc +++ b/src/python/module.cc @@ -6,6 +6,7 @@ namespace py = pybind11; 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_s1interval(py::module& m); void bind_s2point(py::module& m); @@ -18,4 +19,5 @@ PYBIND11_MODULE(s2geometry_bindings, m) { bind_r2rect(m); bind_s1interval(m); bind_s2point(m); + bind_s1angle(m); } diff --git a/src/python/s1angle_bindings.cc b/src/python/s1angle_bindings.cc new file mode 100644 index 00000000..f203ac5f --- /dev/null +++ b/src/python/s1angle_bindings.cc @@ -0,0 +1,158 @@ +#include +#include + +#include +#include + +#include "s2/s1angle.h" +#include "s2/s2point.h" + +namespace py = pybind11; + +namespace { + +void MaybeThrowNotNormalized(const S1Angle& angle) { + if (!angle.IsNormalizedAngle()) { + throw py::value_error("Angle " + std::to_string(angle.degrees()) + + " degrees is not in the normalized range (-180, 180]"); + } +} + +} // namespace + +void bind_s1angle(py::module& m) { + py::class_(m, "S1Angle", + "Represents a one-dimensional angle.\n\n" + "The internal representation is a double-precision value in radians,\n" + "so conversion to and from radians is exact. Conversions between\n" + "degrees and radians are not always exact due to floating-point\n" + "arithmetic (e.g. from_degrees(60).degrees() != 60). Use e5/e6/e7\n" + "representations for exact discrete comparisons.\n\n" + "See s2/s1angle.h for comprehensive documentation including exact\n" + "conversion guarantees and edge cases.") + // Constructors + .def(py::init<>(), "Default constructor creates a zero angle") + .def(py::init(), + py::arg("x"), py::arg("y"), + "Construct the angle between two points.\n\n" + "This is also equal to the distance between the points on the\n" + "unit sphere. The points do not need to be normalized.") + + // Factory methods + .def_static("from_radians", &S1Angle::Radians, py::arg("radians"), + "Construct an angle from its measure in radians.\n\n" + "This conversion is exact.") + .def_static("from_degrees", &S1Angle::Degrees, py::arg("degrees"), + "Construct an angle from its measure in degrees.\n\n" + "Note: the round-trip from_degrees(x).degrees() is not\n" + "always exact. For example, from_degrees(60).degrees() != 60.") + .def_static("from_e5", &S1Angle::E5, py::arg("e5"), + "Construct an angle from its E5 representation.\n\n" + "E5 is degrees multiplied by 1e5 and rounded to the\n" + "nearest integer.\n\n" + "Note: E5 does not share the exact conversion guarantees\n" + "of E6/E7. Avoid testing E5 values for exact equality\n" + "with other formats.") + .def_static("from_e6", &S1Angle::E6, py::arg("e6"), + "Construct an angle from its E6 representation.\n\n" + "E6 is degrees multiplied by 1e6 and rounded to the\n" + "nearest integer.\n\n" + "For any integer n: from_degrees(n) == from_e6(1000000 * n).") + .def_static("from_e7", &S1Angle::E7, py::arg("e7"), + "Construct an angle from its E7 representation.\n\n" + "E7 is degrees multiplied by 1e7 and rounded to the\n" + "nearest integer.\n\n" + "For any integer n: from_degrees(n) == from_e7(10000000 * n).") + .def_static("zero", &S1Angle::Zero, "Return a zero angle") + .def_static("infinity", &S1Angle::Infinity, + "Return an angle larger than any finite angle") + + // Geometric operations + .def("radians", &S1Angle::radians, + "Return the angle in radians.\n\n" + "This is the internal representation, so the conversion is exact.") + .def("degrees", &S1Angle::degrees, + "Return the angle in degrees.\n\n" + "Note: from_degrees(x).degrees() is not always exactly x due to\n" + "the intermediate conversion to radians.") + .def("e5", [](const S1Angle& self) { + MaybeThrowNotNormalized(self); + return self.e5(); + }, + "Return the E5 representation (degrees * 1e5, rounded).\n\n" + "The angle must be in the normalized range (-180, 180] degrees.\n" + "Raises ValueError if out of range.") + .def("e6", [](const S1Angle& self) { + MaybeThrowNotNormalized(self); + return self.e6(); + }, + "Return the E6 representation (degrees * 1e6, rounded).\n\n" + "The angle must be in the normalized range (-180, 180] degrees.\n" + "Raises ValueError if out of range.") + .def("e7", [](const S1Angle& self) { + MaybeThrowNotNormalized(self); + return self.e7(); + }, + "Return the E7 representation (degrees * 1e7, rounded).\n\n" + "The angle must be in the normalized range (-180, 180] degrees.\n" + "Raises ValueError if out of range.") + .def("abs", &S1Angle::abs, "Return the absolute value of the angle") + .def("normalized", &S1Angle::Normalized, + "Return the angle normalized to the range (-180, 180] degrees") + .def("sin_cos", [](const S1Angle& self) { + auto sc = self.SinCos(); + return py::make_tuple(sc.sin, sc.cos); + }, + "Return (sin, cos) of the angle as a tuple.\n\n" + "This may be more efficient than calling sin and cos separately.") + + // Operators + .def(py::self == py::self, "Return true if angles are exactly equal") + .def(py::self != py::self, "Return true if angles are not exactly equal") + .def(py::self < py::self, "Return true if this angle is less than other") + .def(py::self > py::self, + "Return true if this angle is greater than other") + .def(py::self <= py::self, + "Return true if this angle is less than or equal to other") + .def(py::self >= py::self, + "Return true if this angle is greater than or equal to other") + .def(-py::self, "Negate angle") + .def(py::self + py::self, "Add two angles") + .def(py::self - py::self, "Subtract two angles") + .def(py::self * double(), "Multiply angle by scalar") + .def("__rmul__", [](const S1Angle& self, double m) { + return m * self; + }, "Multiply angle by scalar (reversed operands)") + .def(py::self / double(), "Divide angle by scalar") + .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") + .def(py::self -= py::self, "In-place subtraction") + .def("__imul__", [](S1Angle& self, double m) -> S1Angle& { + return self *= m; + }, py::arg("m"), "In-place multiplication by scalar") + .def("__itruediv__", [](S1Angle& self, double m) -> S1Angle& { + return self /= m; + }, py::arg("m"), "In-place division by scalar") + + // String representation + .def("__repr__", [](S1Angle a) { + std::ostringstream oss; + oss << "S1Angle(" << a << ")"; + return oss.str(); + }) + .def("__str__", [](S1Angle a) { + std::ostringstream oss; + oss << a; + return oss.str(); + }); + + // Module-level functions + m.def("sin", [](S1Angle a) { return sin(a); }, py::arg("angle"), + "Return the sine of an S1Angle"); + m.def("cos", [](S1Angle a) { return cos(a); }, py::arg("angle"), + "Return the cosine of an S1Angle"); + m.def("tan", [](S1Angle a) { return tan(a); }, py::arg("angle"), + "Return the tangent of an S1Angle"); +} diff --git a/src/python/s1angle_test.py b/src/python/s1angle_test.py new file mode 100644 index 00000000..b601e155 --- /dev/null +++ b/src/python/s1angle_test.py @@ -0,0 +1,291 @@ +"""Tests for S1Angle pybind11 bindings.""" + +import math +import unittest +import s2geometry_pybind as s2 + + +class TestS1Angle(unittest.TestCase): + """Test cases for S1Angle bindings.""" + + # Constructors + + def test_default_constructor(self): + a = s2.S1Angle() + self.assertEqual(a.radians(), 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.S1Angle(x, y) + 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.S1Angle(p, p) + self.assertEqual(a.radians(), 0.0) + + 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.S1Angle(x, y) + self.assertAlmostEqual(a.radians(), math.pi) + + # Factory methods + + def test_from_radians(self): + a = s2.S1Angle.from_radians(math.pi / 4) + self.assertAlmostEqual(a.radians(), math.pi / 4) + + def test_from_degrees(self): + a = s2.S1Angle.from_degrees(45.0) + self.assertAlmostEqual(a.degrees(), 45.0) + + def test_from_e5(self): + a = s2.S1Angle.from_e5(4500000) + self.assertAlmostEqual(a.degrees(), 45.0) + + def test_from_e6(self): + a = s2.S1Angle.from_e6(45000000) + self.assertAlmostEqual(a.degrees(), 45.0) + + def test_from_e7(self): + a = s2.S1Angle.from_e7(450000000) + self.assertAlmostEqual(a.degrees(), 45.0) + + def test_zero(self): + a = s2.S1Angle.zero() + self.assertEqual(a.radians(), 0.0) + + def test_infinity(self): + a = s2.S1Angle.infinity() + self.assertTrue(math.isinf(a.radians())) + self.assertTrue(a.radians() > 0) + + # Geometric operations + + def test_radians(self): + a = s2.S1Angle.from_radians(1.5) + self.assertEqual(a.radians(), 1.5) + + def test_degrees(self): + a = s2.S1Angle.from_degrees(180.0) + self.assertAlmostEqual(a.radians(), math.pi) + self.assertAlmostEqual(a.degrees(), 180.0) + + def test_degrees_radians_conversion(self): + # Exact conversions per the C++ header documentation. + self.assertEqual(s2.S1Angle.from_degrees(180.0).radians(), math.pi) + for k in range(9): + self.assertEqual( + s2.S1Angle.from_degrees(45 * k).radians(), + k * math.pi / 4) + + def test_e5(self): + a = s2.S1Angle.from_degrees(45.0) + self.assertEqual(a.e5(), 4500000) + + def test_e6(self): + a = s2.S1Angle.from_degrees(45.0) + self.assertEqual(a.e6(), 45000000) + + def test_e7(self): + a = s2.S1Angle.from_degrees(45.0) + self.assertEqual(a.e7(), 450000000) + + def test_e5_e6_e7_consistency(self): + # Degrees(n) == E6(1000000 * n) == E7(10000000 * n) for integer n. + for n in [0, 1, 45, 90, 180, -180, -90]: + self.assertEqual(s2.S1Angle.from_degrees(n).radians(), + s2.S1Angle.from_e6(1000000 * n).radians()) + self.assertEqual(s2.S1Angle.from_degrees(n).radians(), + s2.S1Angle.from_e7(10000000 * n).radians()) + + def test_e5_not_normalized_raises(self): + a = s2.S1Angle.from_degrees(270.0) + with self.assertRaises(ValueError) as cm: + a.e5() + self.assertEqual(str(cm.exception), + "Angle 270.000000 degrees is not in the " + "normalized range (-180, 180]") + + def test_e6_not_normalized_raises(self): + a = s2.S1Angle.from_degrees(270.0) + with self.assertRaises(ValueError) as cm: + a.e6() + self.assertEqual(str(cm.exception), + "Angle 270.000000 degrees is not in the " + "normalized range (-180, 180]") + + def test_e7_not_normalized_raises(self): + a = s2.S1Angle.from_degrees(270.0) + with self.assertRaises(ValueError) as cm: + a.e7() + self.assertEqual(str(cm.exception), + "Angle 270.000000 degrees is not in the " + "normalized range (-180, 180]") + + def test_e6_minus_180_not_normalized_raises(self): + # -180 degrees is outside the normalized range (-180, 180]. + a = s2.S1Angle.from_degrees(-180.0) + with self.assertRaises(ValueError) as cm: + a.e6() + self.assertEqual(str(cm.exception), + "Angle -180.000000 degrees is not in the " + "normalized range (-180, 180]") + + def test_e6_plus_180_is_valid(self): + # +180 degrees is inside the normalized range (-180, 180]. + a = s2.S1Angle.from_degrees(180.0) + self.assertEqual(a.e6(), 180000000) + + def test_abs(self): + pos = s2.S1Angle.from_degrees(45.0) + neg = s2.S1Angle.from_degrees(-45.0) + self.assertAlmostEqual(pos.abs().degrees(), 45.0) + self.assertAlmostEqual(neg.abs().degrees(), 45.0) + + def test_abs_zero(self): + a = s2.S1Angle.zero() + self.assertEqual(a.abs().radians(), 0.0) + + def test_normalized(self): + a = s2.S1Angle.from_degrees(270.0) + self.assertAlmostEqual(a.normalized().degrees(), -90.0) + + def test_normalized_negative(self): + a = s2.S1Angle.from_degrees(-270.0) + self.assertAlmostEqual(a.normalized().degrees(), 90.0) + + def test_normalized_minus_180_becomes_180(self): + # Normalized range is (-180, 180]; -180 maps to 180. + a = s2.S1Angle.from_degrees(-180.0) + self.assertAlmostEqual(a.normalized().degrees(), 180.0) + + def test_sin_cos(self): + a = s2.S1Angle.from_degrees(90.0) + s, c = a.sin_cos() + self.assertAlmostEqual(s, 1.0) + self.assertAlmostEqual(c, 0.0) + + def test_sin_cos_zero(self): + a = s2.S1Angle.zero() + s, c = a.sin_cos() + self.assertAlmostEqual(s, 0.0) + self.assertAlmostEqual(c, 1.0) + + # Operators + + def test_equality(self): + a = s2.S1Angle.from_degrees(45.0) + b = s2.S1Angle.from_degrees(45.0) + c = s2.S1Angle.from_degrees(90.0) + self.assertTrue(a == b) + self.assertTrue(a != c) + self.assertFalse(a != b) + self.assertFalse(a == c) + + def test_comparison(self): + small = s2.S1Angle.from_degrees(10.0) + large = s2.S1Angle.from_degrees(20.0) + self.assertTrue(small < large) + self.assertTrue(large > small) + self.assertTrue(small <= large) + self.assertTrue(large >= small) + self.assertTrue(small <= small) + self.assertTrue(small >= small) + self.assertFalse(small > large) + self.assertFalse(large < small) + + def test_negation(self): + a = s2.S1Angle.from_degrees(45.0) + neg = -a + self.assertAlmostEqual(neg.degrees(), -45.0) + + def test_addition(self): + a = s2.S1Angle.from_degrees(30.0) + b = s2.S1Angle.from_degrees(60.0) + result = a + b + self.assertAlmostEqual(result.degrees(), 90.0) + + def test_subtraction(self): + a = s2.S1Angle.from_degrees(90.0) + b = s2.S1Angle.from_degrees(30.0) + result = a - b + self.assertAlmostEqual(result.degrees(), 60.0) + + def test_scalar_multiplication(self): + a = s2.S1Angle.from_degrees(45.0) + result1 = a * 2.0 + result2 = 2.0 * a + self.assertAlmostEqual(result1.degrees(), 90.0) + self.assertAlmostEqual(result2.degrees(), 90.0) + + def test_scalar_division(self): + a = s2.S1Angle.from_degrees(90.0) + result = a / 2.0 + self.assertAlmostEqual(result.degrees(), 45.0) + + def test_angle_division(self): + a = s2.S1Angle.from_degrees(90.0) + b = s2.S1Angle.from_degrees(45.0) + ratio = a / b + self.assertAlmostEqual(ratio, 2.0) + + def test_in_place_addition(self): + a = s2.S1Angle.from_degrees(30.0) + a += s2.S1Angle.from_degrees(60.0) + self.assertAlmostEqual(a.degrees(), 90.0) + + def test_in_place_subtraction(self): + a = s2.S1Angle.from_degrees(90.0) + a -= s2.S1Angle.from_degrees(30.0) + self.assertAlmostEqual(a.degrees(), 60.0) + + def test_in_place_scalar_multiplication(self): + a = s2.S1Angle.from_degrees(45.0) + a *= 2.0 + self.assertAlmostEqual(a.degrees(), 90.0) + + def test_in_place_scalar_division(self): + a = s2.S1Angle.from_degrees(90.0) + a /= 2.0 + self.assertAlmostEqual(a.degrees(), 45.0) + + # String representation + + def test_repr(self): + a = s2.S1Angle.from_degrees(45.0) + self.assertEqual(repr(a), "S1Angle(45.0000000)") + + def test_str(self): + a = s2.S1Angle.from_degrees(45.0) + self.assertEqual(str(a), "45.0000000") + + def test_repr_zero(self): + a = s2.S1Angle.zero() + self.assertEqual(repr(a), "S1Angle(0.0000000)") + + # Module-level functions + + def test_sin(self): + a = s2.S1Angle.from_degrees(90.0) + self.assertAlmostEqual(s2.sin(a), 1.0) + + def test_cos(self): + a = s2.S1Angle.from_degrees(0.0) + self.assertAlmostEqual(s2.cos(a), 1.0) + + def test_tan(self): + a = s2.S1Angle.from_degrees(45.0) + self.assertAlmostEqual(s2.tan(a), 1.0) + + def test_sin_cos_consistency(self): + a = s2.S1Angle.from_degrees(60.0) + s, c = a.sin_cos() + self.assertAlmostEqual(s2.sin(a), s) + self.assertAlmostEqual(s2.cos(a), c) + + +if __name__ == "__main__": + unittest.main() diff --git a/src/s2/s1angle.h b/src/s2/s1angle.h index c37e9515..63bd7161 100644 --- a/src/s2/s1angle.h +++ b/src/s2/s1angle.h @@ -130,6 +130,10 @@ class S1Angle { int32_t e6() const; int32_t e7() const; + // Returns true if the angle is in the normalized range (-180, 180] + // degrees, which is the valid input range for e5(), e6(), and e7(). + bool IsNormalizedAngle() const; + // Return the absolute value of an angle. S1Angle abs() const; friend S1Angle abs(S1Angle a); @@ -241,7 +245,7 @@ inline constexpr double S1Angle::degrees() const { // between Degrees, E6, and E7 are exact when the arguments are integers. inline int32_t S1Angle::e5() const { - // TODO(user,b/298298095): Tighten this to [-180, 180]. + // TODO(user,b/298298095): Tighten this to (-180, 180] (see IsNormalizedAngle). ABSL_DCHECK_LE(std::numeric_limits::min() / 1e5, degrees()); ABSL_DCHECK_LE(degrees(), std::numeric_limits::max() / 1e5); return MathUtil::Round(1e5 * degrees()); @@ -259,6 +263,10 @@ inline int32_t S1Angle::e7() const { return MathUtil::Round(1e7 * degrees()); } +inline bool S1Angle::IsNormalizedAngle() const { + return radians() > -M_PI && radians() <= M_PI; +} + inline S1Angle S1Angle::abs() const { return S1Angle(std::fabs(radians_)); } From 4e9ae55e58da8e9dd35e95d3b85050c77f846006 Mon Sep 17 00:00:00 2001 From: David Eustis Date: Fri, 10 Apr 2026 15:31:27 +0000 Subject: [PATCH 2/3] Address PR feedback: rename IsNormalizedAngle, drop sin_cos, add __abs__ --- src/python/s1angle_bindings.cc | 11 +++-------- src/python/s1angle_test.py | 23 +++-------------------- src/s2/s1angle.h | 12 ++++++------ 3 files changed, 12 insertions(+), 34 deletions(-) diff --git a/src/python/s1angle_bindings.cc b/src/python/s1angle_bindings.cc index f203ac5f..b7328349 100644 --- a/src/python/s1angle_bindings.cc +++ b/src/python/s1angle_bindings.cc @@ -12,7 +12,7 @@ namespace py = pybind11; namespace { void MaybeThrowNotNormalized(const S1Angle& angle) { - if (!angle.IsNormalizedAngle()) { + if (!angle.IsNormalized()) { throw py::value_error("Angle " + std::to_string(angle.degrees()) + " degrees is not in the normalized range (-180, 180]"); } @@ -96,15 +96,10 @@ void bind_s1angle(py::module& m) { "Return the E7 representation (degrees * 1e7, rounded).\n\n" "The angle must be in the normalized range (-180, 180] degrees.\n" "Raises ValueError if out of range.") - .def("abs", &S1Angle::abs, "Return the absolute value of the angle") + .def("__abs__", &S1Angle::abs, + "Return the absolute value of the angle") .def("normalized", &S1Angle::Normalized, "Return the angle normalized to the range (-180, 180] degrees") - .def("sin_cos", [](const S1Angle& self) { - auto sc = self.SinCos(); - return py::make_tuple(sc.sin, sc.cos); - }, - "Return (sin, cos) of the angle as a tuple.\n\n" - "This may be more efficient than calling sin and cos separately.") // Operators .def(py::self == py::self, "Return true if angles are exactly equal") diff --git a/src/python/s1angle_test.py b/src/python/s1angle_test.py index b601e155..3efa02b7 100644 --- a/src/python/s1angle_test.py +++ b/src/python/s1angle_test.py @@ -142,12 +142,12 @@ def test_e6_plus_180_is_valid(self): def test_abs(self): pos = s2.S1Angle.from_degrees(45.0) neg = s2.S1Angle.from_degrees(-45.0) - self.assertAlmostEqual(pos.abs().degrees(), 45.0) - self.assertAlmostEqual(neg.abs().degrees(), 45.0) + self.assertAlmostEqual(abs(pos).degrees(), 45.0) + self.assertAlmostEqual(abs(neg).degrees(), 45.0) def test_abs_zero(self): a = s2.S1Angle.zero() - self.assertEqual(a.abs().radians(), 0.0) + self.assertEqual(abs(a).radians(), 0.0) def test_normalized(self): a = s2.S1Angle.from_degrees(270.0) @@ -162,18 +162,6 @@ def test_normalized_minus_180_becomes_180(self): a = s2.S1Angle.from_degrees(-180.0) self.assertAlmostEqual(a.normalized().degrees(), 180.0) - def test_sin_cos(self): - a = s2.S1Angle.from_degrees(90.0) - s, c = a.sin_cos() - self.assertAlmostEqual(s, 1.0) - self.assertAlmostEqual(c, 0.0) - - def test_sin_cos_zero(self): - a = s2.S1Angle.zero() - s, c = a.sin_cos() - self.assertAlmostEqual(s, 0.0) - self.assertAlmostEqual(c, 1.0) - # Operators def test_equality(self): @@ -280,11 +268,6 @@ def test_tan(self): a = s2.S1Angle.from_degrees(45.0) self.assertAlmostEqual(s2.tan(a), 1.0) - def test_sin_cos_consistency(self): - a = s2.S1Angle.from_degrees(60.0) - s, c = a.sin_cos() - self.assertAlmostEqual(s2.sin(a), s) - self.assertAlmostEqual(s2.cos(a), c) if __name__ == "__main__": diff --git a/src/s2/s1angle.h b/src/s2/s1angle.h index 63bd7161..c8c603d3 100644 --- a/src/s2/s1angle.h +++ b/src/s2/s1angle.h @@ -130,10 +130,6 @@ class S1Angle { int32_t e6() const; int32_t e7() const; - // Returns true if the angle is in the normalized range (-180, 180] - // degrees, which is the valid input range for e5(), e6(), and e7(). - bool IsNormalizedAngle() const; - // Return the absolute value of an angle. S1Angle abs() const; friend S1Angle abs(S1Angle a); @@ -195,6 +191,10 @@ class S1Angle { // calling sin() and cos() separately. SinCosPair SinCos() const; + // Returns true if the angle is in the normalized range (-180, 180] + // degrees, which is the valid input range for e5(), e6(), and e7(). + bool IsNormalized() const; + // Return the angle normalized to the range (-180, 180] degrees. S1Angle Normalized() const; @@ -245,7 +245,7 @@ inline constexpr double S1Angle::degrees() const { // between Degrees, E6, and E7 are exact when the arguments are integers. inline int32_t S1Angle::e5() const { - // TODO(user,b/298298095): Tighten this to (-180, 180] (see IsNormalizedAngle). + // TODO(user,b/298298095): Tighten this to (-180, 180] (see IsNormalized). ABSL_DCHECK_LE(std::numeric_limits::min() / 1e5, degrees()); ABSL_DCHECK_LE(degrees(), std::numeric_limits::max() / 1e5); return MathUtil::Round(1e5 * degrees()); @@ -263,7 +263,7 @@ inline int32_t S1Angle::e7() const { return MathUtil::Round(1e7 * degrees()); } -inline bool S1Angle::IsNormalizedAngle() const { +inline bool S1Angle::IsNormalized() const { return radians() > -M_PI && radians() <= M_PI; } From 34c27c7a3909e1b8e774cba85675b4d586e14ddb Mon Sep 17 00:00:00 2001 From: David Eustis Date: Tue, 21 Apr 2026 16:03:57 +0000 Subject: [PATCH 3/3] Address PR feedback: properties, absl::StrCat, instance trig methods - Make radians/degrees/e5/e6/e7 properties instead of methods - Expose IsNormalized() and Normalize() - Replace std::to_string with absl::StrCat (locale safety) - Move sin/cos/tan from module-level functions to instance methods - Organize module.cc into dependency tranches with comments - Update README property rule --- src/python/README.md | 4 +- src/python/module.cc | 12 +- src/python/s1angle_bindings.cc | 60 +++++----- src/python/s1angle_test.py | 183 ++++++++++++++++-------------- src/python/s1interval_bindings.cc | 3 +- 5 files changed, 146 insertions(+), 116 deletions(-) diff --git a/src/python/README.md b/src/python/README.md index 37208f4a..5da9b0af 100644 --- a/src/python/README.md +++ b/src/python/README.md @@ -36,9 +36,9 @@ The Python bindings follow the C++ API closely but with Pythonic conventions: - Method names are converted to snake_case (converted from UpperCamelCase C++ function names) **Properties vs. Methods:** -- Simple coordinate accessors are properties: `point.x`, `point.y`, `interval.lo`, `interval.hi` +- Simple accessors that return internal state (including trivial unit conversions) are properties: `point.x`, `point.y`, `interval.lo`, `interval.hi`, `angle.radians`, `angle.degrees` - Properties are always read-only. To create a modified object, use a constructor or factory method. -- Other functions are not properties: `angle.radians()`, `angle.degrees()`, `interval.length()` +- Other functions are methods: `interval.length()`, `angle.normalized()`, `angle.sin()` **Invalid Values:** - Invalid inputs to constructions or functions raises `ValueError`. diff --git a/src/python/module.cc b/src/python/module.cc index 40256ed5..13f6ebe0 100644 --- a/src/python/module.cc +++ b/src/python/module.cc @@ -13,11 +13,19 @@ void bind_s2point(py::module& m); PYBIND11_MODULE(s2geometry_bindings, m) { m.doc() = "S2 Geometry Python bindings using pybind11"; - // Bind core geometry classes in dependency order + // Bind core geometry classes in dependency order. + // Each class must be registered before classes that use it as a + // parameter or return type. + + // No dependencies bind_r1interval(m); bind_r2point(m); - bind_r2rect(m); bind_s1interval(m); bind_s2point(m); + + // Deps: r1interval, r2point + bind_r2rect(m); + + // Deps: s2point bind_s1angle(m); } diff --git a/src/python/s1angle_bindings.cc b/src/python/s1angle_bindings.cc index b7328349..0e30422f 100644 --- a/src/python/s1angle_bindings.cc +++ b/src/python/s1angle_bindings.cc @@ -1,9 +1,10 @@ #include #include +#include #include -#include +#include "absl/strings/str_cat.h" #include "s2/s1angle.h" #include "s2/s2point.h" @@ -13,8 +14,9 @@ namespace { void MaybeThrowNotNormalized(const S1Angle& angle) { if (!angle.IsNormalized()) { - throw py::value_error("Angle " + std::to_string(angle.degrees()) + - " degrees is not in the normalized range (-180, 180]"); + throw py::value_error( + absl::StrCat("Angle ", angle.degrees(), + " degrees is not in the normalized range (-180, 180]")); } } @@ -26,7 +28,7 @@ void bind_s1angle(py::module& m) { "The internal representation is a double-precision value in radians,\n" "so conversion to and from radians is exact. Conversions between\n" "degrees and radians are not always exact due to floating-point\n" - "arithmetic (e.g. from_degrees(60).degrees() != 60). Use e5/e6/e7\n" + "arithmetic (e.g. from_degrees(60).degrees != 60). Use e5/e6/e7\n" "representations for exact discrete comparisons.\n\n" "See s2/s1angle.h for comprehensive documentation including exact\n" "conversion guarantees and edge cases.") @@ -44,8 +46,8 @@ void bind_s1angle(py::module& m) { "This conversion is exact.") .def_static("from_degrees", &S1Angle::Degrees, py::arg("degrees"), "Construct an angle from its measure in degrees.\n\n" - "Note: the round-trip from_degrees(x).degrees() is not\n" - "always exact. For example, from_degrees(60).degrees() != 60.") + "Note: the round-trip from_degrees(x).degrees is not\n" + "always exact. For example, from_degrees(60).degrees != 60.") .def_static("from_e5", &S1Angle::E5, py::arg("e5"), "Construct an angle from its E5 representation.\n\n" "E5 is degrees multiplied by 1e5 and rounded to the\n" @@ -67,39 +69,53 @@ void bind_s1angle(py::module& m) { .def_static("infinity", &S1Angle::Infinity, "Return an angle larger than any finite angle") - // Geometric operations - .def("radians", &S1Angle::radians, - "Return the angle in radians.\n\n" + // Properties + .def_property_readonly("radians", &S1Angle::radians, + "The angle in radians.\n\n" "This is the internal representation, so the conversion is exact.") - .def("degrees", &S1Angle::degrees, - "Return the angle in degrees.\n\n" - "Note: from_degrees(x).degrees() is not always exactly x due to\n" + .def_property_readonly("degrees", &S1Angle::degrees, + "The angle in degrees.\n\n" + "Note: from_degrees(x).degrees is not always exactly x due to\n" "the intermediate conversion to radians.") - .def("e5", [](const S1Angle& self) { + .def_property_readonly("e5", [](const S1Angle& self) { MaybeThrowNotNormalized(self); return self.e5(); }, - "Return the E5 representation (degrees * 1e5, rounded).\n\n" + "The E5 representation (degrees * 1e5, rounded).\n\n" "The angle must be in the normalized range (-180, 180] degrees.\n" "Raises ValueError if out of range.") - .def("e6", [](const S1Angle& self) { + .def_property_readonly("e6", [](const S1Angle& self) { MaybeThrowNotNormalized(self); return self.e6(); }, - "Return the E6 representation (degrees * 1e6, rounded).\n\n" + "The E6 representation (degrees * 1e6, rounded).\n\n" "The angle must be in the normalized range (-180, 180] degrees.\n" "Raises ValueError if out of range.") - .def("e7", [](const S1Angle& self) { + .def_property_readonly("e7", [](const S1Angle& self) { MaybeThrowNotNormalized(self); return self.e7(); }, - "Return the E7 representation (degrees * 1e7, rounded).\n\n" + "The E7 representation (degrees * 1e7, rounded).\n\n" "The angle must be in the normalized range (-180, 180] degrees.\n" "Raises ValueError if out of range.") + + // Predicates + .def("is_normalized", &S1Angle::IsNormalized, + "Return true if the angle is in the normalized range (-180, 180]") + + // Geometric operations .def("__abs__", &S1Angle::abs, "Return the absolute value of the angle") .def("normalized", &S1Angle::Normalized, "Return the angle normalized to the range (-180, 180] degrees") + .def("normalize", &S1Angle::Normalize, + "Normalize this angle in-place to the range (-180, 180] degrees") + .def("sin", [](const S1Angle& self) { return sin(self); }, + "Return the sine of the angle") + .def("cos", [](const S1Angle& self) { return cos(self); }, + "Return the cosine of the angle") + .def("tan", [](const S1Angle& self) { return tan(self); }, + "Return the tangent of the angle") // Operators .def(py::self == py::self, "Return true if angles are exactly equal") @@ -142,12 +158,4 @@ void bind_s1angle(py::module& m) { oss << a; return oss.str(); }); - - // Module-level functions - m.def("sin", [](S1Angle a) { return sin(a); }, py::arg("angle"), - "Return the sine of an S1Angle"); - m.def("cos", [](S1Angle a) { return cos(a); }, py::arg("angle"), - "Return the cosine of an S1Angle"); - m.def("tan", [](S1Angle a) { return tan(a); }, py::arg("angle"), - "Return the tangent of an S1Angle"); } diff --git a/src/python/s1angle_test.py b/src/python/s1angle_test.py index 3efa02b7..110fed5f 100644 --- a/src/python/s1angle_test.py +++ b/src/python/s1angle_test.py @@ -12,155 +12,177 @@ class TestS1Angle(unittest.TestCase): def test_default_constructor(self): a = s2.S1Angle() - self.assertEqual(a.radians(), 0.0) + self.assertEqual(a.radians, 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.S1Angle(x, y) - self.assertAlmostEqual(a.radians(), math.pi / 2) + 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.S1Angle(p, p) - self.assertEqual(a.radians(), 0.0) + self.assertEqual(a.radians, 0.0) 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.S1Angle(x, y) - self.assertAlmostEqual(a.radians(), math.pi) + self.assertAlmostEqual(a.radians, math.pi) # Factory methods def test_from_radians(self): a = s2.S1Angle.from_radians(math.pi / 4) - self.assertAlmostEqual(a.radians(), math.pi / 4) + self.assertAlmostEqual(a.radians, math.pi / 4) def test_from_degrees(self): a = s2.S1Angle.from_degrees(45.0) - self.assertAlmostEqual(a.degrees(), 45.0) + self.assertAlmostEqual(a.degrees, 45.0) def test_from_e5(self): a = s2.S1Angle.from_e5(4500000) - self.assertAlmostEqual(a.degrees(), 45.0) + self.assertAlmostEqual(a.degrees, 45.0) def test_from_e6(self): a = s2.S1Angle.from_e6(45000000) - self.assertAlmostEqual(a.degrees(), 45.0) + self.assertAlmostEqual(a.degrees, 45.0) def test_from_e7(self): a = s2.S1Angle.from_e7(450000000) - self.assertAlmostEqual(a.degrees(), 45.0) + self.assertAlmostEqual(a.degrees, 45.0) def test_zero(self): a = s2.S1Angle.zero() - self.assertEqual(a.radians(), 0.0) + self.assertEqual(a.radians, 0.0) def test_infinity(self): a = s2.S1Angle.infinity() - self.assertTrue(math.isinf(a.radians())) - self.assertTrue(a.radians() > 0) + self.assertTrue(math.isinf(a.radians)) + self.assertTrue(a.radians > 0) - # Geometric operations + # Properties def test_radians(self): a = s2.S1Angle.from_radians(1.5) - self.assertEqual(a.radians(), 1.5) + self.assertEqual(a.radians, 1.5) def test_degrees(self): a = s2.S1Angle.from_degrees(180.0) - self.assertAlmostEqual(a.radians(), math.pi) - self.assertAlmostEqual(a.degrees(), 180.0) + self.assertAlmostEqual(a.radians, math.pi) + self.assertAlmostEqual(a.degrees, 180.0) def test_degrees_radians_conversion(self): # Exact conversions per the C++ header documentation. - self.assertEqual(s2.S1Angle.from_degrees(180.0).radians(), math.pi) + self.assertEqual(s2.S1Angle.from_degrees(180.0).radians, math.pi) for k in range(9): self.assertEqual( - s2.S1Angle.from_degrees(45 * k).radians(), + s2.S1Angle.from_degrees(45 * k).radians, k * math.pi / 4) def test_e5(self): a = s2.S1Angle.from_degrees(45.0) - self.assertEqual(a.e5(), 4500000) + self.assertEqual(a.e5, 4500000) def test_e6(self): a = s2.S1Angle.from_degrees(45.0) - self.assertEqual(a.e6(), 45000000) + self.assertEqual(a.e6, 45000000) def test_e7(self): a = s2.S1Angle.from_degrees(45.0) - self.assertEqual(a.e7(), 450000000) + self.assertEqual(a.e7, 450000000) def test_e5_e6_e7_consistency(self): # Degrees(n) == E6(1000000 * n) == E7(10000000 * n) for integer n. for n in [0, 1, 45, 90, 180, -180, -90]: - self.assertEqual(s2.S1Angle.from_degrees(n).radians(), - s2.S1Angle.from_e6(1000000 * n).radians()) - self.assertEqual(s2.S1Angle.from_degrees(n).radians(), - s2.S1Angle.from_e7(10000000 * n).radians()) + self.assertEqual(s2.S1Angle.from_degrees(n).radians, + s2.S1Angle.from_e6(1000000 * n).radians) + self.assertEqual(s2.S1Angle.from_degrees(n).radians, + s2.S1Angle.from_e7(10000000 * n).radians) + + def test_properties_are_readonly(self): + a = s2.S1Angle.from_degrees(45.0) + with self.assertRaises(AttributeError): + a.radians = 1.0 + with self.assertRaises(AttributeError): + a.degrees = 1.0 def test_e5_not_normalized_raises(self): a = s2.S1Angle.from_degrees(270.0) - with self.assertRaises(ValueError) as cm: - a.e5() - self.assertEqual(str(cm.exception), - "Angle 270.000000 degrees is not in the " - "normalized range (-180, 180]") + with self.assertRaises(ValueError): + _ = a.e5 def test_e6_not_normalized_raises(self): a = s2.S1Angle.from_degrees(270.0) - with self.assertRaises(ValueError) as cm: - a.e6() - self.assertEqual(str(cm.exception), - "Angle 270.000000 degrees is not in the " - "normalized range (-180, 180]") + with self.assertRaises(ValueError): + _ = a.e6 def test_e7_not_normalized_raises(self): a = s2.S1Angle.from_degrees(270.0) - with self.assertRaises(ValueError) as cm: - a.e7() - self.assertEqual(str(cm.exception), - "Angle 270.000000 degrees is not in the " - "normalized range (-180, 180]") + with self.assertRaises(ValueError): + _ = a.e7 def test_e6_minus_180_not_normalized_raises(self): # -180 degrees is outside the normalized range (-180, 180]. a = s2.S1Angle.from_degrees(-180.0) - with self.assertRaises(ValueError) as cm: - a.e6() - self.assertEqual(str(cm.exception), - "Angle -180.000000 degrees is not in the " - "normalized range (-180, 180]") + with self.assertRaises(ValueError): + _ = a.e6 def test_e6_plus_180_is_valid(self): # +180 degrees is inside the normalized range (-180, 180]. a = s2.S1Angle.from_degrees(180.0) - self.assertEqual(a.e6(), 180000000) + self.assertEqual(a.e6, 180000000) + + # Predicates + + def test_is_normalized(self): + self.assertTrue(s2.S1Angle.from_degrees(45.0).is_normalized()) + self.assertTrue(s2.S1Angle.from_degrees(180.0).is_normalized()) + self.assertFalse(s2.S1Angle.from_degrees(270.0).is_normalized()) + self.assertFalse(s2.S1Angle.from_degrees(-180.0).is_normalized()) + + # Geometric operations def test_abs(self): pos = s2.S1Angle.from_degrees(45.0) neg = s2.S1Angle.from_degrees(-45.0) - self.assertAlmostEqual(abs(pos).degrees(), 45.0) - self.assertAlmostEqual(abs(neg).degrees(), 45.0) + self.assertAlmostEqual(abs(pos).degrees, 45.0) + self.assertAlmostEqual(abs(neg).degrees, 45.0) def test_abs_zero(self): a = s2.S1Angle.zero() - self.assertEqual(abs(a).radians(), 0.0) + self.assertEqual(abs(a).radians, 0.0) def test_normalized(self): a = s2.S1Angle.from_degrees(270.0) - self.assertAlmostEqual(a.normalized().degrees(), -90.0) + self.assertAlmostEqual(a.normalized().degrees, -90.0) def test_normalized_negative(self): a = s2.S1Angle.from_degrees(-270.0) - self.assertAlmostEqual(a.normalized().degrees(), 90.0) + self.assertAlmostEqual(a.normalized().degrees, 90.0) def test_normalized_minus_180_becomes_180(self): # Normalized range is (-180, 180]; -180 maps to 180. a = s2.S1Angle.from_degrees(-180.0) - self.assertAlmostEqual(a.normalized().degrees(), 180.0) + self.assertAlmostEqual(a.normalized().degrees, 180.0) + + def test_normalize_in_place(self): + a = s2.S1Angle.from_degrees(270.0) + a.normalize() + self.assertAlmostEqual(a.degrees, -90.0) + + def test_sin(self): + a = s2.S1Angle.from_degrees(90.0) + self.assertAlmostEqual(a.sin(), 1.0) + + def test_cos(self): + a = s2.S1Angle.from_degrees(0.0) + self.assertAlmostEqual(a.cos(), 1.0) + + def test_tan(self): + a = s2.S1Angle.from_degrees(45.0) + self.assertAlmostEqual(a.tan(), 1.0) # Operators @@ -188,31 +210,31 @@ def test_comparison(self): def test_negation(self): a = s2.S1Angle.from_degrees(45.0) neg = -a - self.assertAlmostEqual(neg.degrees(), -45.0) + self.assertAlmostEqual(neg.degrees, -45.0) def test_addition(self): a = s2.S1Angle.from_degrees(30.0) b = s2.S1Angle.from_degrees(60.0) result = a + b - self.assertAlmostEqual(result.degrees(), 90.0) + self.assertAlmostEqual(result.degrees, 90.0) def test_subtraction(self): a = s2.S1Angle.from_degrees(90.0) b = s2.S1Angle.from_degrees(30.0) result = a - b - self.assertAlmostEqual(result.degrees(), 60.0) + self.assertAlmostEqual(result.degrees, 60.0) def test_scalar_multiplication(self): a = s2.S1Angle.from_degrees(45.0) result1 = a * 2.0 result2 = 2.0 * a - self.assertAlmostEqual(result1.degrees(), 90.0) - self.assertAlmostEqual(result2.degrees(), 90.0) + self.assertAlmostEqual(result1.degrees, 90.0) + self.assertAlmostEqual(result2.degrees, 90.0) def test_scalar_division(self): a = s2.S1Angle.from_degrees(90.0) result = a / 2.0 - self.assertAlmostEqual(result.degrees(), 45.0) + self.assertAlmostEqual(result.degrees, 45.0) def test_angle_division(self): a = s2.S1Angle.from_degrees(90.0) @@ -223,51 +245,42 @@ def test_angle_division(self): def test_in_place_addition(self): a = s2.S1Angle.from_degrees(30.0) a += s2.S1Angle.from_degrees(60.0) - self.assertAlmostEqual(a.degrees(), 90.0) + self.assertAlmostEqual(a.degrees, 90.0) def test_in_place_subtraction(self): a = s2.S1Angle.from_degrees(90.0) a -= s2.S1Angle.from_degrees(30.0) - self.assertAlmostEqual(a.degrees(), 60.0) + self.assertAlmostEqual(a.degrees, 60.0) def test_in_place_scalar_multiplication(self): a = s2.S1Angle.from_degrees(45.0) a *= 2.0 - self.assertAlmostEqual(a.degrees(), 90.0) + self.assertAlmostEqual(a.degrees, 90.0) def test_in_place_scalar_division(self): a = s2.S1Angle.from_degrees(90.0) a /= 2.0 - self.assertAlmostEqual(a.degrees(), 45.0) + self.assertAlmostEqual(a.degrees, 45.0) # String representation def test_repr(self): - a = s2.S1Angle.from_degrees(45.0) - self.assertEqual(repr(a), "S1Angle(45.0000000)") + # The repr format comes from C++ operator<< which uses %.7f on degrees. + self.assertEqual(repr(s2.S1Angle.from_degrees(45.0)), + "S1Angle(45.0000000)") + self.assertEqual(repr(s2.S1Angle.from_degrees(45.00)), + "S1Angle(45.0000000)") + self.assertEqual(repr(s2.S1Angle.from_degrees(0.0)), + "S1Angle(0.0000000)") + self.assertEqual(repr(s2.S1Angle.from_degrees(-90.0)), + "S1Angle(-90.0000000)") + # Precision beyond 7 decimal places is truncated. + self.assertEqual(repr(s2.S1Angle.from_degrees(45.0000000001)), + "S1Angle(45.0000000)") def test_str(self): - a = s2.S1Angle.from_degrees(45.0) - self.assertEqual(str(a), "45.0000000") - - def test_repr_zero(self): - a = s2.S1Angle.zero() - self.assertEqual(repr(a), "S1Angle(0.0000000)") - - # Module-level functions - - def test_sin(self): - a = s2.S1Angle.from_degrees(90.0) - self.assertAlmostEqual(s2.sin(a), 1.0) - - def test_cos(self): - a = s2.S1Angle.from_degrees(0.0) - self.assertAlmostEqual(s2.cos(a), 1.0) - - def test_tan(self): - a = s2.S1Angle.from_degrees(45.0) - self.assertAlmostEqual(s2.tan(a), 1.0) - + self.assertEqual(str(s2.S1Angle.from_degrees(45.0)), "45.0000000") + self.assertEqual(str(s2.S1Angle.zero()), "0.0000000") if __name__ == "__main__": diff --git a/src/python/s1interval_bindings.cc b/src/python/s1interval_bindings.cc index e4db7da8..49454275 100644 --- a/src/python/s1interval_bindings.cc +++ b/src/python/s1interval_bindings.cc @@ -3,6 +3,7 @@ #include +#include "absl/strings/str_cat.h" #include "s2/s1interval.h" namespace py = pybind11; @@ -11,7 +12,7 @@ namespace { void MaybeThrowInvalidPoint(double p) { if (!S1Interval::IsValidPoint(p)) { - throw py::value_error("Invalid S1 point: " + std::to_string(p)); + throw py::value_error(absl::StrCat("Invalid S1 point: ", p)); } }