pybind: Add S2CellId iteration (children, cells, ranges)#640
Open
deustis wants to merge 12 commits into
Open
Conversation
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.
Moves the S2CellIdRange, S2CellIdForwardIter, and S2CellIdReverseIter types out of the Python bindings and into the s2 library as first-class C++ types. The C++ layer uses std::optional<S2CellId> instead of throwing; all exception handling is consolidated in the bindings behind named helpers (RangeLenOrThrow, RangeItemOrThrow, RangeSliceOrThrow, NextOrStop). Adds a gtest suite covering size(), at(), slice(), contains(), and both iterator types.
- Rename S2CellIdForwardIter/ReverseIter to …Iterator - at(): return S2CellId directly (precondition: i in [0,size())), add ABSL_DCHECK; bindings validate bounds before calling - slice(): add ABSL_DCHECKs for 0<=start<=stop<=size() preconditions - RangeLenOrThrow: add 32-bit platform hint to overflow message - RangeItemOrThrow: check bounds directly, handle double-negative index - Drop tests for invalid at() inputs (now DCHECK territory)
- Rename helpers to take primitive inputs and return validated values; range method calls (at(), slice()) stay in the binding lambdas - RangeLenOrThrow -> LenOrThrow - RangeItemOrThrow -> SliceIndexOrThrow(i, n) -> int64_t - ComputeSlice + RangeSliceOrThrow -> SliceBoundsOrThrow(n, s) -> pair - Add Python data model doc links to SliceIndexOrThrow and SliceBoundsOrThrow - Fix RangeItemOrThrow comment to say "0-based index" - Use short-circuit in ValidateSliceStepOrThrow
- Copyright year 2024 -> 2026 in new files - Revert Traversal section update in README.md - SliceIndexOrThrow -> IndexOrThrow - ValidateSliceStepOrThrow -> StepOrThrow - SliceBoundsOrThrow -> ClampedSlice (pure computation, no step validation); binding now calls StepOrThrow directly before ClampedSlice - Update slice doc URL to #sequences
- Rename s2cellid_range.{h,cc,_test.cc} to s2cell_id_range.* to match
the s2cell_id_* naming convention used throughout the library.
- Fix children(level) to reject level == self.level(); the lower bound
was self.level() so child_begin(self.level()) returned self, not its
children.
- Fix test_children_of_leaf_raises expected message to match what
MaybeThrowIfLeaf actually emits.
- Add test_children_at_own_level_raises to cover the level-bound fix.
- Replace S2CellIdReverseIterator's done-flag design with a sentinel:
cur starts at range.end and decrements before yielding, so empty
ranges work without a special case and __reversed__ becomes a
one-liner.
- Add ABSL_DCHECK in size() that begin and end are at the same level.
- Rename iterator Python classes to _S2CellId{Forward,Reverse}Iterator
to signal they are implementation details, not public API.
deustis
commented
Jun 16, 2026
deustis
commented
Jun 16, 2026
jmr
requested changes
Jul 10, 2026
jmr
left a comment
jmr
left a comment
Member
There was a problem hiding this comment.
I think it's a good approach. More info in the PR would have made my life easier. problem/solution/alternatives considered/why this is best.
https://google.github.io/eng-practices/review/developer/cl-descriptions.html#informative
- Move s2cell_id_range.h/.cc and test from src/s2/ to src/python/ per reviewer request (implementation detail of Python bindings, not public API) - Add file-level comment to s2cell_id_range.h - Fix copyright to "Google LLC" (drop "All Rights Reserved") - Update include guard and include paths - Add comment explaining why py::slice::compute() is not used (Py_ssize_t is 32-bit on 32-bit platforms; S2CellIdRange needs int64_t) - Fix alignment in ClampedSlice - Add __eq__ and __repr__ to S2CellIdRange pybind binding - Use std::optional<S2CellId> instead of auto in test iterators - Rename reverse iterator variable to rit
Contributor
Author
Ack. Updated |
Contributor
Author
|
@jmr, thanks for the review! All comments should be addressed |
Contributor
Author
jmr
reviewed
Jul 17, 2026
| // with forward and reverse iterators. These types are exposed to pybind11 to | ||
| // support Python's sequence protocol (len, indexing, slicing, iteration, | ||
| // reversed) on S2CellId ranges returned by methods such as children() and | ||
| // cells_at_level(). |
Fix method name in file comment: cells_at_level() -> cells().
Member
|
tests just started failing and i don't see why |
Contributor
Author
|
I'm not able to repro locally. I'm suspecting the e1cc10f commit bumped the required abseil to 20260526.0, which moved throw_delegate.h to a new public location. I have an older local abseil... |
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.
Add Python iteration support for S2CellId hierarchy traversal. Two new methods are exposed:
Both return an S2CellIdRange, which supports the Python sequence protocol: len(), integer indexing, contiguous slicing (step=1), in, for, and reversed(). Slicing and indexing are O(1) via S2CellId::advance() and distance_from_begin().
S2CellIdRange is a lazy view rather than a list because the ranges can be very large (a face cell's children(30) has ~10^9 elements). It supports len(), indexing, and slicing — not just iteration — because those are natural operations on a contiguous range of cells and can all be implemented in O(1). The range helpers live in src/python/ as an implementation detail of the bindings and are not part of the public C++ API.