pybind: Add S2Cap bindings#646
Open
deustis wants to merge 12 commits into
Open
Conversation
Binds S2Cap to Python, including constructors, static factories (from_point, from_center_height, from_center_area, empty, full), properties (center, radius, height), predicates (is_valid, is_empty, is_full), geometric ops (complement, expanded, union), containment and intersection methods, mutation (add_point, add_cap), and the S2Region interface (get_cap_bound, cell_union_bound). Also resolves the TODO in s2cell_bindings.cc by wiring up S2Cell.get_cap_bound() now that S2Cap is bound before S2Cell in the module initialization order. get_rect_bound() on both S2Cap and S2Cell remains deferred pending S2LatLngRect bindings.
- Fix __repr__ to prefix class name per README convention - Rename get_radius/get_area/get_centroid to radius_angle/area/centroid - Rename get_cap_bound to cap_bound (drop Get prefix per convention) - Add __hash__ so S2Cap can be used in sets and as dict keys - Add @abseil-cpp//absl/hash BUILD dep - Fix module.cc deps comment (s2cell is not a dep of s2cap) - Update tests for renamed methods - Add __hash__ test and approx_equals false-case test
Mutating methods are inconsistent with the otherwise-immutable API. from_points and from_caps provide the same bounding-region-from-collection pattern without mutating the receiver.
S2Cap only depends on s1angle, s1chord_angle, and s2point, all of which are already bound at that point. Order bindings by fewest dependencies.
Invalid caps are now impossible to construct from Python: both S2Point constructors raise ValueError for non-unit-length centers. is_valid() is removed as it would always return True.
Static factories -> Factory methods; fold Computed accessors and S2Region interface labels into Geometric operations.
Silently normalizing is more ergonomic: callers working with approximate unit vectors don't need to pre-normalize, and it matches the behavior of S2Cap::FromPoint and other factories.
from_point, from_center_height, and from_center_area now normalize the center point consistent with the constructors, so callers don't need to pre-normalize.
No s2cell changes belong in the s2cap PR; they will land with s2region.
- Plain section headers matching the bindings (no dashes) - Add class docstring - Drop assertIsInstance; use value assertions throughout - Consolidate orphaned sub-sections into Geometric operations / Operators
Contributor
Author
|
@jmr, this one is also ready for review |
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
S2Cap.Notable design choices:
center,radius,height)rect_boundis deferred untilS2LatLngRectis bound.from_pointsandfrom_capsreplace the mutableAddPoint/AddCappattern — callers pass a list and get back a new bounding cap.(Part of a series addressing #524)
Test plan
bazel test //python:s2cap_test //python:s2cell_test— all tests pass.