pybind: Add S2LatLngRect bindings

[deustis]

Adds pybind11 bindings for S2LatLngRect, covering all key C++ methods including the S2Region interface methods (cap_bound, rect_bound, cell_union_bound).

Design choices:

• from_latlngs: custom factory replacing the mutable AddPoint pattern — callers pass a list of S2LatLng and receive a new bounding rectangle
• distance / distance_latlng: renamed from the GetDistance overloads (dropping _to_rect / _to_latlng per naming convention)
• cell_union_bound: wraps GetCellUnionBound's output parameter, returning a list instead
• directed_hausdorff_distance: uses py::overload_cast to resolve the public/private overload ambiguity in C++
• approx_equals: only the (S2LatLngRect, S1Angle) overload is bound; the interval-level overload is omitted

Test plan

bazel test //python:s2latlng_rect_test //python:s2cell_test — all tests pass.

[deustis]

@jmr, this PR had some failing tests for awhile, fixed now and ready for review.

[jmr]

I see lots of things to improve. You can try a prompt like this: Review #655 for correctness and idiomaticity of the pybind11 bindings and generated Python code and consistency with existing bindings. we should be generating Pythonic interfaces. anything that would DCHECK in C++ should be handled by exceptions

[deustis]

I see lots of things to improve.

Hmm, any hints? I tried a few round of that already. Found one DCHECK issue with boundary_intersects but otherwise my AI didn't have much to say.

[jmr]

Sorry, Google hates PATs, so there's not a good way to post these programmatically without using even less secure auth methods (don't ask my why they hate PATs).

1. Correctness, Invariants, and ABSL_DCHECK / ABSL_DLOG Issues

1. rect.size() breaks the Python S2LatLng validity invariant (and causes downstream ABSL_DCHECK crashes) (s2latlng_rect_bindings.cc:99-104, 171-173)

   • In s2latlng_bindings.cc:36-38, all S2LatLng instances in Python are guaranteed to be valid (lat in [-90, 90], lng in [-180, 180] degrees). Both s2cell_bindings.cc:68-69 and
     s2latlng_rect_bindings.cc rely on this invariant and skip ll.is_valid() checks on S2LatLng inputs.
   • However, S2LatLngRect::GetSize() returns S2LatLng::FromRadians(lat_.GetLength(), lng_.GetLength()).
   • For S2LatLngRect.full().size(), this returns an invalid S2LatLng(180°, 360°); for S2LatLngRect.empty().size(), it returns an invalid S2LatLng(-1 rad, -360°); and for any rectangle
     spanning > 90° in latitude or > 180° in longitude, rect.size() returns an invalid S2LatLng.
   • Passing the S2LatLng returned by rect.size() to S2LatLngRect(lo, hi), from_point_pair, from_latlngs, contains_latlng, interior_contains_latlng, or distance_latlng triggers
     ABSL_DCHECK(IsValidPoint(p)) / ABSL_DCHECK(is_valid()) crashes in s1interval.cc / s1interval.h, and passing it to from_point constructs an invalid S2LatLngRect.
   • Conversely, because S2LatLng constructors in Python reject lat outside [-90, 90] and lng outside [-180, 180], callers cannot construct an S2LatLng with lat > 90° or lng > 180° to
     pass as size to from_center_size(center, size) or as margin to expanded(margin) — contradicting the docstring on lines 102-104 ("center must be normalized; size does not need to be... and the longitude interval is Full() if the longitude size is 360 degrees or more").

2. S2LatLngRect(lo, hi) allows constructing an invalid rectangle when lo.lng == 180° and hi.lng == -180° (s2latlng_rect_bindings.cc:39-43, 78-86)

   • MaybeThrowLatNotOrdered only checks lo.lat() > hi.lat().
   • If lo = S2LatLng.from_degrees(0, 180) and hi = S2LatLng.from_degrees(10, -180), lo.lat() <= hi.lat() holds, but S1Interval(M_PI, -M_PI) is S1Interval::Empty(). This produces a rectangle with
     non-empty lat_ and empty lng_, which is invalid (!rect.is_valid()) and logs ABSL_DLOG_IF(ERROR, !is_valid()).

3. MaybeThrowInvalidLatInterval lets NaN through (s2latlng_rect_bindings.cc:26-30, 87-96)

   • Line 27 checks if (std::fabs(lat.lo()) > M_PI_2 || std::fabs(lat.hi()) > M_PI_2).
   • R1Interval(float('nan'), float('nan')) can be constructed in Python. For NaN, std::fabs(NaN) > M_PI_2 and lat.is_empty() (NaN > NaN) are both false, so S2LatLngRect(R1Interval(nan, nan), S1Interval(0, 1)) succeeds and creates an invalid S2LatLngRect (whereas C++ is_valid() uses std::fabs(lat_.lo()) <= M_PI_2 && std::fabs(lat_.hi()) <= M_PI_2, which is false for NaN).

4. Non-canonical empty R1Interval in S2LatLngRect(lat, lng) breaks Python's == / __hash__ contract (s2latlng_rect_bindings.cc:87-96, 309-311)

   • If a caller passes a non-canonical empty R1Interval such as lat = R1Interval(0.5, -0.5) with lng = S1Interval.empty(), S2LatLngRect(lat, lng) stores lat_ = [0.5, -0.5] instead of canonical
     R1Interval::Empty() ([1, 0]).
   • rect == S2LatLngRect.empty() evaluates to True (because R1Interval::operator== treats all empty intervals as equal), but hash(rect) != hash(S2LatLngRect.empty()) because AbslHashValue hashes
     rect.lo() and rect.hi() directly.
   • Also, if a caller passes R1Interval(2.0, 1.0) (an empty R1Interval whose dummy endpoints exceed pi/2) and S1Interval.empty(), MaybeThrowInvalidLatInterval rejects it even though both
     intervals are empty. Returning S2LatLngRect::Empty() when lat.is_empty() && lng.is_empty() (before checking endpoint bounds) avoids non-canonical empty rectangles.

5. contains_point and interior_contains_point crash on ABSL_DCHECK with non-finite S2Points (s2latlng_rect_bindings.cc:189-202)

   • S2Point(float('inf'), float('inf'), 0.0) can be constructed in Python.
   • In S2LatLngRect::Contains(const S2Point&) and InteriorContains(const S2Point&), S2LatLng(p) computes lat = atan2(0, inf) = 0 and lng = atan2(inf, inf) = NaN. When lat_.Contains(0) is true,
     lng_.Contains(NaN) / lng_.InteriorContains(NaN) crashes on ABSL_DCHECK(IsValidPoint(p)) in s1interval.cc:72 / 79.
   • Compare s2latlng_bindings.cc:54-62, where S2LatLng(const S2Point&) checks MaybeThrowNotValid(ll).

6. expanded_by_distance with Empty() or non-finite / out-of-range S1Angle (s2latlng_rect_bindings.cc:258-263)

   • If distance is S1Angle.from_radians(float('nan')) and rect has lng = FullLng(), ExpandedByDistance takes the else branch (s2latlng_rect.cc:264-295) and returns
     S2LatLngRect(R1Interval(NaN, NaN), FullLng()), which is invalid (!is_valid()) and logs ABSL_DLOG(ERROR).
   • If distance is < -Pi (e.g. S1Angle.from_degrees(-200)), sin(-distance.radians()) in s2latlng_rect.cc:286 becomes negative, causing lng().Expanded(-max_lng_margin) to expand instead of
     shrink.
   • Also note a C++ bug in s2latlng_rect.cc:252-263: S2LatLngRect::Empty().ExpandedByDistance(d) for d >= 0 does not check if (is_empty()) return Empty();, so it builds caps around the dummy
     vertices of Empty() (lat = 1, 0, lng = ±pi) and returns a non-empty rectangle, contradicting s2latlng_rect.h:271-273 ("The full and empty rectangles have no boundary on the sphere. Any expansion (positive or negative) of these rectangles leaves them unchanged.").

2. Consistency with Existing Bindings and Conventions

1. vertex(k) rejects k outside [0, 3] instead of reducing modulo 4 (s2latlng_rect_bindings.cc:51-55, 161-167)
   • s2latlng_rect.h:147-150 explicitly documents: "For convenience, the argument is reduced modulo 4 to the range [0..3]." (and s2latlng_rect.cc:79-84 has no DCHECK and uses bitwise masking `(k >>

1. & 1andk & 1`).

• Both R2Rect.vertex(k) (r2rect_bindings.cc:89-95) and S2Cell.vertex(k) (s2cell_bindings.cc:114-118) bind &GetVertex directly and document that k is reduced modulo 4 (which enables the
  standard S2 idiom (rect.vertex(k), rect.vertex(k + 1)) for iterating the 4 edges).
• MaybeThrowVertexOutOfRange should be removed and &S2LatLngRect::GetVertex bound directly.

2. Deferred rect_bound() in S2Cap and S2Cell was not wired up

   • s2cap_bindings.cc:152 has // get_rect_bound() is deferred until S2LatLngRect is bound.
   • s2cell_bindings.cc:248-251 has a TODO for get_cap_bound() and get_rect_bound() waiting on S2Cap and S2LatLngRect.
   • Also in module.cc:60, the // Deps: comment for bind_s2latlng_rect omits s2cap (used by cap_bound).

3. Docstrings copy-paste C++ preconditions that do not apply to Python (s2latlng_rect_bindings.cc:85, 102, 107, 112, 188, 207, 276)

   • Multiple docstrings tell the caller "Both points must be normalized", "The argument must be normalized", or "The point must be valid" for S2LatLng arguments.
   • In Python, S2LatLng has neither .is_valid() nor .normalized() because S2LatLng is guaranteed valid/normalized on construction. Compare s2cell_bindings.cc:67-73, which omits this from the
     Python docstring and instead notes in a C++ comment that all S2LatLng objects from Python are valid.

4. Section ordering and method ordering (s2latlng_rect_bindings.cc:180-212, 303-308)

   • Per src/python/README.md and existing bindings (r1interval_bindings.cc:95, s1interval_bindings.cc:127, r2rect_bindings.cc:160, s2latlng_bindings.cc:137):
     • approx_equals belongs under // Geometric operations (with // Note: default value must match C++ signature in s2latlng_rect.h), not inside // Operators between != and __hash__.
     • contains* is ordered (rect, latlng, point, cell) (lines 180-197), while interior_contains* right below it is ordered (point, latlng, rect) (lines 198-212).

5. Parameter naming consistency

   • contains_latlng and interior_contains_latlng name the S2LatLng parameter ll, while distance_latlng (line 274) and from_point (line 106) name the S2LatLng parameter p (and
     distance_latlng's docstring calls it "the given point" instead of S2LatLng).
   • contains_point and interior_contains_point use py::arg("p"), whereas s2cap_bindings.cc:128 and s2cell_bindings.cc:199 use py::arg("point").

6. Omitted ApproxEquals overload and AddPoint(const S2Point&) equivalent

   • The PR description states: "approx_equals: only the (S2LatLngRect, S1Angle) overload is bound; the interval-level overload is omitted." The second overload in s2latlng_rect.h:328 is
     ApproxEquals(const S2LatLngRect& other, const S2LatLng& max_error) (separate lat and lng tolerances, not an interval).
   • S2LatLngRect has both AddPoint(const S2LatLng&) and AddPoint(const S2Point&) in C++ (s2latlng_rect.h:217-218); only from_latlngs is provided (no from_points taking S2Points as in S2Cap. from_points).

3. C++ and pybind11 Idiomaticity

1. Simplify from_latlngs (s2latlng_rect_bindings.cc:121-128)

   if (latlngs.empty()) return S2LatLngRect::Empty();
   S2LatLngRect result = S2LatLngRect::FromPoint(latlngs[0]);
   for (size_t i = 1; i < latlngs.size(); ++i) result.AddPoint(latlngs[i]);
   return result;

   S2LatLngRect() is already Empty(), and AddPoint on an empty rectangle initializes it to that point (s2latlng_rect.h:99-100). This can simply be:

   S2LatLngRect result;
   for (const S2LatLng& ll : latlngs) {
     result.AddPoint(ll);
   }
   return result;

2. Duplicated helper without comment (MaybeThrowNotUnitLength) (s2latlng_rect_bindings.cc:57-63)
   • MaybeThrowNotUnitLength is copied verbatim from s1chord_angle_bindings.cc:18-24. In s2cell_bindings.cc:21, duplicated helpers have a comment noting where they are duplicated from.
3. Include offending values in ValueError messages (s2latlng_rect_bindings.cc:26-49)
   • MaybeThrowInvalidLatInterval, MaybeThrowEmptyMismatch, MaybeThrowLatNotOrdered, and MaybeThrowIfEmpty use static strings without formatting the offending values (unlike r2rect_bindings.cc:18- 21, s2latlng_bindings.cc:19-21, and s2cell_id_bindings.cc:22).

4. Test Coverage and Quality (s2latlng_rect_test.py)

Many tests in s2latlng_rect_test.py only check a weak property (like not is_empty(), > 0.0, or True on full()) rather than testing exact results, False cases, and edge cases:

1. test_constructor_from_lo_hi / test_constructor_from_intervals / test_lo_hi (s2latlng_rect_test.py:18-34, 136-142):
   • test_constructor_from_lo_hi and test_constructor_from_intervals only assert self.assertFalse(rect.is_empty()) without verifying rect.lo == lo, rect.hi == hi, rect.lat == lat, rect.lng == lng.
   • test_lo_hi only checks rect.lo.lat.degrees and rect.hi.lat.degrees, omitting lng (or self.assertEqual(rect.lo, lo) / self.assertEqual(rect.hi, hi)).
   • Missing tests for constructing from two empty intervals (S2LatLngRect(R1Interval.empty(), S1Interval.empty())), non-empty lat with empty lng, and lat.lo < -pi/2.
2. test_area (s2latlng_rect_test.py:211-213):
   • Asserts self.assertGreater(s2.S2LatLngRect.full().area(), 0.0) instead of self.assertAlmostEqual(s2.S2LatLngRect.full().area(), 4 * math.pi) (compare s2cap_test.py:152).
3. test_polar_closure_of_full_is_full (s2latlng_rect_test.py:323-324):
   • Only tests full().polar_closure(), which is a no-op since full() already has FullLng(). It should test a rectangle touching a pole with a partial longitude range (e.g. lat=[80, 90], lng=[10, 20] -> lng becomes FullLng()) and a non-polar rectangle remaining unchanged (as in s2latlng_rect_test.cc:416-425).
4. One-sided predicate tests (missing False cases):
   • test_contains_point, test_contains_cell, test_interior_contains_point, test_interior_contains_rect, test_intersects_cell, test_interior_intersects, test_may_intersect, and
     test_approx_equals only assert True (often just against S2LatLngRect.full() or rect against itself), never testing False cases or boundary vs. interior distinctions.
5. Trivial distance and set-operation assertions:
   • test_distance (lines 354-362) only asserts d.radians > 0.0 instead of the known 10.0 degree distance.
   • test_distance_latlng (lines 372-378) only tests a point inside the rectangle (d == 0), never an exterior point.
   • test_directed_hausdorff_distance and test_hausdorff_distance (lines 385-398) only test 0.0 distance cases (r2 inside full(), and rect to itself).
   • test_expanded and test_expanded_by_distance (lines 315-321, 346-352) only check expanded.contains(rect), without checking the resulting bounds or negative margins/distances (shrinking).
   • test_from_point_pair (lines 62-67) passes already-ordered points (-10, -20) and (10, 20) instead of testing unordered points or antimeridian wrapping.
   • Missing readonly property tests (with self.assertRaises(AttributeError): rect.lo = ..., compare r2rect_test.py:102-112).

[deustis]

Wow! I believe I've addressed those comments, and tried a few more prompts to squeeze a few more as well. PTAL!