pybind: Add R2 bindings - #546
Conversation
Add pybind11 bindings for S1Interval. Also add interface notes to the python README
|
This is a child of PR #534. Diffs are showing both PRs... not sure how to set the upstream to my fork. Leaving this in draft until we land the upstream. |
| if (x_empty != y_empty) { | ||
| throw py::value_error( | ||
| "Invalid R2Rect: x and y intervals must be both empty or both " | ||
| "non-empty"); |
jmr
left a comment
There was a problem hiding this comment.
I'm also not sure about the "expose all the things" approach. We've never exposed R2* in the old SWIG or new google3 bindings and no one complained.
Maybe you want to do a useful subset / the SWIG subset first.
| "order.\n\n" | ||
| "Vertex 0 is in the lower-left corner. The argument is reduced " | ||
| "modulo 4.") | ||
| .def("vertex_ij", |
There was a problem hiding this comment.
A single overloaded vertex might make sense. I'm not sure and don't have strong feelings.
There was a problem hiding this comment.
I left a note about this here:
https://github.com/google/s2geometry/pull/546/changes#diff-68b4b8a36a193cbacbd9e072b0026059a3554ed10ee9779d07856a8110628193R61
Python overloads are generally frowned on but if you prefer to match the C++ behavior I could get behind that too. Or we could drop this method as it is pretty obscure.
Merge branch 'master' into deustis/r2_bindings. Add pybind11 bindings for R1Interval, R2Point, and R2Rect. (Part of a series of addressing google#524.)
8a5bb2b to
f3c5782
Compare
I believe How about this?: Let's try to land this PR and then I'll move on to some more useful classes like |
|
@jwr, please see above and let me know how you'd like to proceed?: |
|
|
||
| **Method Overloads:** | ||
| - Some methods accept more than one type for the same argument. For example, `R2Rect.contains()` accepts either an `R2Point` or another `R2Rect`. | ||
| - When a C++ class has overloads with significantly different behavior, the Python bindings use distinct method names instead. For example, `R2Rect` provides `vertex(k)` for CCW vertex index and `vertex_ij(i, j)` for axis-direction vertex access. |
There was a problem hiding this comment.
Not for this PR, but if we consider these "significantly different", we should rename them in C++, too. I was thinking of them as pretty much the same, but it could make sense to rename in C++.
|
One thing would be to support |
|
@jmr, friendly nudge, let me know you'd like to see any other changes on this? |
|
Sorry about the delay. Thanks for the contribution! |
Add pybind11 bindings for R1Interval, R2Point, and R2Rect
(Part of a series of addressing #524)