pybind: Add S2LatLngRect bindings - #655
Open
deustis wants to merge 14 commits into
Open
Conversation
Binds S2LatLngRect to Python with full coverage of the public API: constructors, static factories (from_center_size, from_point, from_point_pair, empty, full, full_lat, full_lng), properties (lat, lng, lat_lo/hi, lng_lo/hi, lo, hi), predicates (is_valid, is_empty, is_full, is_point, is_inverted), geometric accessors (vertex, center, size, get_area, get_centroid), containment and intersection methods, mutation (add_point), set operations (expanded, polar_closure, union, intersection, expanded_by_distance), distance methods (distance_to_rect, distance_to_latlng, directed_hausdorff_distance, hausdorff_distance), the S2Region interface (get_rect_bound, cell_union_bound), and operators (==, !=, approx_equals, approx_equals_latlng, __hash__, __repr__, __str__). Also resolves the TODO in s2cell_bindings.cc by wiring up S2Cell.get_rect_bound() now that S2LatLngRect is bound after S2Cell in the module initialization order. get_cap_bound() on S2LatLngRect and S2Cell remains deferred pending S2Cap bindings.
- Fix __repr__ to prefix class name per README convention - Rename get_area/get_centroid to area/centroid (drop Get prefix) - Rename get_rect_bound to rect_bound (drop Get prefix per convention) - Add inverted rect test (antimeridian-spanning) - Improve boundary_intersects test coverage - Move test_s2cell_rect_bound to s2cell_test.py
Mutating methods are inconsistent with the otherwise-immutable API. from_latlngs provides the same bounding-rect-from-collection pattern without mutating the receiver.
The R1Interval+S1Interval constructor now raises ValueError for out-of-range lat or empty/non-empty mismatch between lat and lng. is_valid() is removed as it would always return True for objects reachable from Python.
Static factories -> Factory methods; Geometric accessors -> Geometric operations; remove S2Region interface label.
- Extract throw conditions into MaybeThrowInvalidLatInterval and MaybeThrowEmptyMismatch helpers, matching the pattern in other bindings - Rename distance_to_rect -> distance, distance_to_latlng -> distance_latlng to match the same-type-short-name convention used by contains/intersects - Drop approx_equals_latlng (the S2LatLng-tolerance overload is not compelling enough to expose under a separate name) - Collapse non-README subsection headers into Geometric operations; add // String representation section comment - Add comment on from_latlngs explaining it replaces the mutable AddPoint pattern (the Python interface is immutable) - Add comment on cell_union_bound explaining the output-parameter conversion - Add test class docstring; fix test_str to assert content; cross-check both == and != in test_equality/test_inequality; rename test methods to match renamed bindings; use plain # Section style throughout
- Move inline comments from before .def() calls into lambda bodies - Replace assertIsInstance calls with concrete value assertions
deustis
marked this pull request as ready for review
July 17, 2026 21:20
S2CellId.is_valid() is intentionally not exposed in the Python bindings; the binding contract guarantees validity by raising ValueError on invalid inputs. Replace the per-element is_valid() loop with an isinstance check.
deustis
force-pushed
the
deustis/s2latlngrect
branch
from
August 18, 2026 05:41
4c8bcf5 to
f2b5a73
Compare
Contributor
Author
|
@jmr, this PR had some failing tests for awhile, fixed now and ready for review. |
Add anonymous namespace around helpers (consistent with all other bindings files). Add three new guard helpers: - MaybeThrowLatNotOrdered: raised by the (lo, hi) constructor when lo.lat() > hi.lat(), which would otherwise silently produce an internally inconsistent rect. - MaybeThrowIfEmpty: raised by distance() and distance_latlng() when the rect is empty, replacing an ABSL_DCHECK that aborts in debug builds and produces garbage in release. - MaybeThrowVertexOutOfRange: raised by vertex() for k outside [0, 3], avoiding implementation-defined signed right-shift on negative k. Add tests for each new error path. Remove redundant is_inverted() assertion from test_inverted_rect (already covered by test_is_inverted).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 mutableAddPointpattern — callers pass a list ofS2LatLngand receive a new bounding rectangledistance/distance_latlng: renamed from theGetDistanceoverloads (dropping_to_rect/_to_latlngper naming convention)cell_union_bound: wrapsGetCellUnionBound's output parameter, returning a list insteaddirected_hausdorff_distance: usespy::overload_castto resolve the public/private overload ambiguity in C++approx_equals: only the(S2LatLngRect, S1Angle)overload is bound; the interval-level overload is omittedTest plan
bazel test //python:s2latlng_rect_test //python:s2cell_test— all tests pass.