pybind: Add S2Interval bindings (#524) #534
Conversation
deustis
force-pushed
the
deustis/s1interval_bindings
branch
4 times, most recently
from
596f5f6 to
85ae427
Compare
Add pybind11 bindings for S1Interval. Also add interface notes to the python README
deustis
force-pushed
the
deustis/s1interval_bindings
branch
from
85ae427 to
5bb24fa
Compare
|
@jmr, next installment here. I've added some updates to the README that explain some of the user-facing interface choices for the python bindings. That might be a good place to start for the review. Feel free to suggest any changes. |
| pybind_extension( | ||
| name = "s2geometry_bindings", | ||
| srcs = ["module.cc"], | ||
| copts = ["-DNDEBUG"], |
There was a problem hiding this comment.
I'm not sure about this. Definitely needs a comment.
| The Python bindings follow the C++ API closely but with Pythonic conventions: | ||
|
|
||
| **Naming Conventions:** | ||
| - Core classes exist within the top-level module; we intend to define submodules for utility classes. |
There was a problem hiding this comment.
What would these submodules look like? Maybe leave it as "may define", since I don't really know.
| **Properties vs. Methods:** | ||
| - Simple coordinate accessors are properties: `point.x`, `point.y`, `interval.lo`, `interval.hi` | ||
| - If the C++ class has a trivial set_foo method for the property, then the Python property is mutable (otherwise it is a read-only property). | ||
| - Other functions are not properties: `angle.radians()`, `angle.degrees()`, `interval.get_length()` |
| - `S1Interval(0.0, 4.0)` with `4.0 > π` creates interval where `is_valid()` is `False` | ||
| - `S2Point(3.0, 4.0, 0.0)` creates an unnormalized point outside of the unit sphere; use `normalize()` to normalize. | ||
| - In **C++** invalid values will typically trigger (`ABSL_DCHECK`) when compiled with debug options. | ||
| - In **Python bindings** debug assertions (`ABSL_DCHECK`) are disabled, matching the production optimized C++ behavior. |
There was a problem hiding this comment.
Hmm. Why? Isn't this an ODR violation waiting to happen?
| - In **Python bindings** debug assertions (`ABSL_DCHECK`) are disabled, matching the production optimized C++ behavior. | ||
|
|
||
| **Type Conversions:** | ||
| - Python floats map to C++ `double` |
|
|
||
| Use the following sections to organize functions within the bindings files and tests: | ||
|
|
||
| 1. **Constructors** - Default constructors and constructors with parameters |
There was a problem hiding this comment.
This seems reasonable, but it's also helpful if the order reflects the C++ order.
| "Check if interval is inverted (lo > hi)") | ||
|
|
||
| // Geometric operations | ||
| .def("get_center", &S1Interval::GetCenter, "Return center of interval") |
| "Return directed Hausdorff distance to another interval") | ||
| .def("approx_equals", &S1Interval::ApproxEquals, | ||
| py::arg("other"), py::arg("max_error") = 1e-15, | ||
| "Check if approximately equal") |
There was a problem hiding this comment.
Let's make the docstrings more informative. max_error is radians, etc.
Can you tell your LLM to copy the docstrings from C++?
| py::arg("other"), | ||
| "Return directed Hausdorff distance to another interval") | ||
| .def("approx_equals", &S1Interval::ApproxEquals, | ||
| py::arg("other"), py::arg("max_error") = 1e-15, |
There was a problem hiding this comment.
Is there a way for pybind to get the default from C++?
| std::to_string(i.hi()) + ")"; | ||
| }) | ||
| .def("__str__", [](const S1Interval& i) { | ||
| if (i.is_empty()) return std::string("[∅]"); |
There was a problem hiding this comment.
Maybe "[empty]"/"[full]" are more consistent with other places.
I also wonder if we should just delegate to operator<< in C++.
2 participants
Add pybind11 bindings for S1Interval.
Also add interface notes to the python README
(Part of a series of addressing #524)