pybind: Add S2CellId bindings by deustis · Pull Request #593 · google/s2geometry

deustis · 2026-05-04T23:49:35Z

Add pybind11 bindings for S2CellId.

Iteration-like methods (e.g. next(), prev(), Begin(), End()) are not included.

Also updates the README with new binding file organization sections (Constants, Traversal).

(Part of a series addressing #524)

…stent (s,t)-space - Rename kMaxLevel/kNumFaces to MAX_LEVEL/NUM_FACES (upper snake case for static constants) - Use absl::StrCat in MaybeThrowNotValid - Drop to_point_raw, expanded_by_distance_uv, get_center_si_ti, to_face_ij_orientation, to_string - Use "(s,t)-space" and "(u,v)-space" consistently in docstrings - Update README constants section and remove tests for dropped bindings

- Drop from_debug_string binding; tests now construct cells via chained child() calls - Assert exact/approximate values for (s,t), (u,v), and range_min/_max tests instead of type-only or "> 0" checks - Annotate 1 << 30 with context about kMaxLevel - Drop test_children_for_loop (redundant with test_children) - Consolidate S2CellIdRange tests under test_s2cell_id_range_* so the range-class contract is tested in one place rather than duplicated per range-returning factory

deustis · 2026-05-04T23:53:53Z

@jmr, ready for review!

jmr · 2026-05-05T08:29:57Z

for c in cell: / reversed(cell) walks the Hilbert curve

This is unclear to me. Is this children? Leaf cells? I don't think I'd have an interface like this, but could be missing something.

deustis · 2026-05-05T15:23:34Z

for c in cell: / reversed(cell) walks the Hilbert curve

This is unclear to me. Is this children? Leaf cells? I don't think I'd have an interface like this, but could be missing something.

There are several iteration patterns in the C++ class. This self iterable binding provides the same functionality as next/prev:
https://github.com/google/s2geometry/blob/master/src/s2/s2cell_id.h#L348

  // Return the next/previous cell at the same level along the Hilbert curve.
  // Works correctly when advancing from one face to the next, but
  // does *not* wrap around from the last face to the first or vice versa.
  IFNDEF_SWIG([[nodiscard]]) S2CellId next() const;
  IFNDEF_SWIG([[nodiscard]]) S2CellId prev() const;

The functionality is very similar to Begin/End except it starts at the current cell rather than at the first cell in a level:

  // Iterator-style methods for traversing all the cells along the Hilbert
  // curve at a given level (across all 6 faces of the cube).  Note that the
  // end value is exclusive (just like standard STL iterators), and is not a
  // valid cell id.
  static S2CellId Begin(int level);
  static S2CellId End(int level);

(This one is bound as the static cells method)

I tried to make these iteration patterns more consistent and pythonic with S2CellIdRange and S2CellIdForwardIter. Very much open to feedback on whether we should expose them all and also on naming.

jmr · 2026-05-07T07:54:06Z

How many more PRs do you have stacked up behind this one? An easy way might be to remove the iteration, go on with the rest and come back to the iteration later.

jmr · 2026-05-07T08:09:19Z

The functionality is very similar to Begin/End except it starts at the current cell rather than at the first cell in a level:

So it does a third thing that I didn't guess. range(cell, end(cell.level)) will work here. I think we'd want to see how often this comes up before having a special solution.

…tatic Drop S2CellId.__iter__ / __reversed__ and the S2CellIdRange / iterator types; these move to a follow-up PR per jmr's suggestion. Also simplify MAX_LEVEL / NUM_FACES from def_property_readonly_static lambdas to def_readonly_static.

deustis · 2026-05-07T17:05:04Z

@jmr, took your suggestion to split iteration into a follow-up. Should be ready for another review pass

Bring back a focused Traversal section in README covering hierarchy navigation (parent, child, neighbors). Move test_range_min_max_*, test_contains, test_intersects, and test_get_common_ancestor_level* under # Geometric operations to match the bindings file and README.

jmr · 2026-05-08T06:58:44Z

Ask your favorite LLM to do a code review. Ask if the pybind11 code and generated Python bindings are idiomatic.

jmr · 2026-05-08T07:04:24Z

+8. **Vector operations** - Methods from the Vector base class (e.g., `norm`, `norm2`, `normalize`, `dot_prod`, `cross_prod`, `angle`). Only applicable to classes that inherit from `util/math/vector.h`
+9. **Operators** - Operator overloads (e.g., `==`, `+`, `*`, comparison operators)
+10. **String representation** - `__repr__` (which also provides `__str__`), and string conversion methods like `to_string_in_degrees`
+11. **Module-level functions** - Standalone functions (e.g., trigonometric functions for S1Angle)


This is getting too long and too hard to see the diffs. Use "1." everywhere.

https://google.github.io/styleguide/docguide/style.html#use-lazy-numbering-for-long-lists

I will update the existing items.

deustis · 2026-05-08T23:11:34Z

Idiomaticity review (from Claude, per jmr's suggestion):

Overall: the bindings are well-structured. Naming conventions (snake_case methods, UPPER_SNAKE constants), exception types (ValueError throughout), constructor overloading, and the tuple-vs-list distinction for fixed/variable-size neighbor returns are all idiomatic. Docstrings are appropriate and consistent with the other binding files.

Three items worth considering:

1. face, level, pos could be properties instead of methods.
The project convention uses properties for cheap O(1) accessors: S2Point.x, S1Angle.radians, S2LatLng.lat, and cell.id is already a property in this file. Having cell.id as a property but cell.level() as a method is inconsistent. cell.face, cell.level, cell.pos would match both the project convention and Python user expectations.

2. __repr__ format is not eval-able.
repr(cell) returns S2CellId(0/) which looks like it should round-trip through eval() but can't. Python convention says repr should either be eval-able or use angle-bracket notation. Using the numeric id (S2CellId(1152921504606846976)) would actually round-trip via the uint64 constructor. However, the current approach is consistent with the other bindings in this project (S2Point, S1Angle, S2LatLng all wrap C++ operator<< output), so this may not be worth changing unless all bindings are updated together.

3. is_leaf/is_face — methods vs properties.
These are cheap bit-checks on an immutable value, so they'd work as properties. But S1Angle.is_normalized() and similar predicates in the project are methods. Keeping them as methods is consistent and defensible.

- Make face, level, pos into read-only properties (consistent with S2Point.x, S1Angle.radians, S2LatLng.lat, and cell.id) - Add i/j range validation to from_face_ij via new MaybeThrowCellIdOutOfRange helper - Fix get_vertex_neighbors on face cells (level-0 made range [0, -1]) - Generalize MaybeThrowIfFace/MaybeThrowIfLeaf error messages - Switch README numbered list to lazy numbering per style guide

deustis · 2026-05-08T23:17:46Z

face, level, pos could be properties instead of methods. The project convention uses properties for cheap O(1) accessors: S2Point.x, S1Angle.radians, S2LatLng.lat, and cell.id is already a property in this file. Having cell.id as a property but cell.level() as a method is inconsistent. cell.face, cell.level, cell.pos would match both the project convention and Python user expectations.

Switched to properties for those methods, skipped the other suggestions.

deustis · 2026-05-08T23:18:26Z

@jmr, I think this is ready for another round

# Conflicts: # src/python/BUILD.bazel

jmr · 2026-05-11T20:15:45Z

+      .def("get_size_st",
+           py::overload_cast<>(&S2CellId::GetSizeST, py::const_),
+           "Return the edge length of this cell in (s,t)-space")
+      .def_static("get_size_st_for_level", [](int level) {


Done. Added test_get_size_st_for_level covering level 0 (full unit interval) and level 30.

jmr · 2026-05-11T20:17:02Z

+        self.assertLessEqual(len(neighbors), 4)
+
+    def test_get_vertex_neighbors_level_too_high_raises(self):
+        cell = s2.S2CellId.from_face(0)


have separate test cases for face and level, since you have separate checks for these

Done. Split into test_get_vertex_neighbors_face_cell_raises (exercises MaybeThrowIfFace) and test_get_vertex_neighbors_level_out_of_range_raises (exercises MaybeThrowLevelOutOfRange with a level-3 cell).

jmr · 2026-05-11T20:23:36Z


 1. **Constructors** - Default constructors and constructors with parameters
 1. **Factory methods** - Static factory methods (e.g., `from_degrees`, `from_radians`, `zero`, `invalid`)
+1. **Constants** - Class-level constants in upper snake case (e.g., `S2CellId.MAX_LEVEL`, `S2CellId.NUM_FACES`)


constants first?

Done. Moved Constants before Factory methods in the README, matching the order already used in the binding file.

- Add test for get_size_st_for_level - Split get_vertex_neighbors error tests into separate face and level cases - Move Constants before Factory methods in README binding file organization

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.)

Add S2CellId.children() and S2CellId.cells() returning S2CellIdRange, a pythonic sequence over cells at a common Hilbert-curve level with __len__, __getitem__ (int + slice), __contains__, __iter__, and __reversed__. Design notes from PR google#593 review: - __len__ narrows int64 count to Py_ssize_t and raises OverflowError when the range exceeds Py_ssize_t::max (can happen on 32-bit Python for ranges above cells(15)). Users needing the full count on such platforms should use S2CellIdRange.size(). - __getitem__ supports integer indexing and slicing. Slices with step != 1 raise ValueError since they cannot be represented as a contiguous range; reversed() covers the step=-1 case. - S2CellIdForwardIter / S2CellIdReverseIter are marked module_local, since they are implementation details of the iteration protocol and should not collide across pybind modules.

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.)

Add S2CellId.children() and S2CellId.cells() returning S2CellIdRange, a pythonic sequence over cells at a common Hilbert-curve level with __len__, __getitem__ (int + slice), __contains__, __iter__, and __reversed__. Design notes from PR google#593 review: - __len__ narrows int64 count to Py_ssize_t and raises OverflowError when the range exceeds Py_ssize_t::max (can happen on 32-bit Python for ranges above cells(15)). Users needing the full count on such platforms should use S2CellIdRange.size(). - __getitem__ supports integer indexing and slicing. Slices with step != 1 raise ValueError since they cannot be represented as a contiguous range; reversed() covers the step=-1 case. - S2CellIdForwardIter / S2CellIdReverseIter are marked module_local, since they are implementation details of the iteration protocol and should not collide across pybind modules.

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.)

Add S2CellId.children() and S2CellId.cells() returning S2CellIdRange, a pythonic sequence over cells at a common Hilbert-curve level with __len__, __getitem__ (int + slice), __contains__, __iter__, and __reversed__. Design notes from PR google#593 review: - __len__ narrows int64 count to Py_ssize_t and raises OverflowError when the range exceeds Py_ssize_t::max (can happen on 32-bit Python for ranges above cells(15)). Users needing the full count on such platforms should use S2CellIdRange.size(). - __getitem__ supports integer indexing and slicing. Slices with step != 1 raise ValueError since they cannot be represented as a contiguous range; reversed() covers the step=-1 case. - S2CellIdForwardIter / S2CellIdReverseIter are marked module_local, since they are implementation details of the iteration protocol and should not collide across pybind modules.

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.)

deustis added 4 commits May 4, 2026 22:31

pybind: Add S2CellId bindings

3d106e3

Cascade style changes: absl::StrCat, properties, dep comments

5afd0d6

Merge branch 'master' into deustis/s2cell_id_bindings

f8fa72f

This was referenced May 7, 2026

pybind: Add S2Cell bindings deustis/s2geometry#4

Closed

pybind: Add S1ChordAngle bindings deustis/s2geometry#5

Closed