pybind: Add S2Cell bindings - #631
Merged
Merged
Conversation
Binds S2Cell's constructors, factory methods, geometric accessors (vertex, edge, center, area), boundary helpers, containment and intersection predicates, and subdivide. Adds 43 unit tests. Distance methods and the S2Cap/S2LatLngRect-returning region methods are deferred with a TODO; they depend on S1ChordAngle/S2Cap/S2LatLngRect bindings which are not yet in place. (Stacked on deustis/s2cell_id_bindings / PR google#593.)
- Drop MaybeThrowNotValidCellId from S2Cell(S2CellId) constructor; all S2CellId objects reachable from Python are already validated - Remove instance average_area(); keep only static average_area_for_level() - Add S2CellId::kMaxPosition constant and validate pos in from_face_pos_level for both S2CellId and S2Cell bindings - Switch constants to cls.attr() pattern in both S2CellId and S2Cell bindings; update README to document this style - Fix BUILD: add absl/strings dep to s2cell_id_bindings - Remove misleading encode/decode comment from s2cell_bindings.cc - Fix magic constant 1<<30 -> 1<<S2CellId.MAX_LEVEL in s2cell_test.py
Consistent with other MaybeThrow* helpers in both binding files.
- Fix s2cell deps comment in module.cc to include s1chord_angle - Clarify orientation docstring (bitmask, values 0-3) - Drop _raw variants (vertex_raw, edge_raw, center_raw): Python callers don't feed unnormalized vectors into exact C++ predicates
Contributor
Author
|
@jmr, I've done a few rounds of self/LLM review here. Happy to make any changes as usual. |
jmr
reviewed
May 29, 2026
jmr
reviewed
May 29, 2026
jmr
reviewed
May 29, 2026
jmr
reviewed
May 29, 2026
jmr
reviewed
May 29, 2026
jmr
reviewed
May 29, 2026
Member
|
Tests are failing. |
- Drop get_ prefix from all Python method names across S2Cell, S2CellId, and S2LatLng bindings; document convention in README - Use py::enum_<S2Cell::Boundary> with py::arithmetic() instead of raw cls.attr() integer assignments - Use absl::HashOf in __hash__; add absl/hash include and BUILD dep - Add validity comment on S2LatLng constructor; comment duplicated helpers - Add tests for all seven distance methods on S2Cell
Contributor
Author
Fixed |
Contributor
Author
|
@jmr, this one is also ready for another review |
jmr
approved these changes
Jun 9, 2026
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
2 participants
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
S2Cell, covering constructors, factory methods, properties, geometric operations, containment/intersection tests, distance queries, and traversal.Naming:
GetDistanceis overloaded in C++ forS2Point, edge(a, b), andS2Cellarguments. Per the binding interface notes, the short name (get_distance) is used for theS2Pointoverload, and longer names (get_distance_to_edge,get_distance_to_cell) for the others.get_max_distancefollows the same pattern.Omitted::
_rawvariants:**vertex_raw,edge_raw, andcenter_rawreturn unnormalized vectors intended for feeding into exact C++ predicates. Python callers won't do that, so these are dropped in favor of the normalized versions.is_distance_less/is_distance_less_or_equalfamily is omitted — callers can useget_distance(...) < limitdirectly.get_cap_boundandget_rect_boundare deferred untilS2CapandS2LatLngRectare bound.Encode/Decodeare intentionally omitted per the serialization policy in the pybind README.(Part of a series addressing #522)
Test plan
bazel test //python/...— all suites pass.