pybind: Add S1Interval 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. |
deustis
changed the title
|
@jmr, thanks for the detailed comments. I think once we iron out some of these interface choices it will be smooth sailing. Definitely worth spending the time up-front to ensure consistency/correctness. I've addressed most of your comments. The main open question is about how to handle the debug assertions. Let me know your thoughts there and I'll make those changes. |
|
Is there a reason you picked |
|
Hi there, just a quick note -- I was traveling last week and working against some deadlines this week but still intending to circle back on this!
No particular reason... starting with the lowest level primitives and working my way up to |
|
@jmr, apologies for the delay. I'm back and have updated the the PR as discussed. The python interface will now raise value errors for invalid inputs to avoid any possibility of triggering the DCHECKS. In the case of S1Angle this basically means ensuring all of the points are within +/- pi. I've added a static helper to the C++ code to make this easier so we aren't having to do any math in the bindings themselves. |
|
@jmr, friendly ping on this PR? I believe I've addressed your earlier comments. |
|
I will take a closer look at this tomorrow. |
src/python/s1interval_bindings.cc
Outdated
| namespace { | ||
|
|
||
| void MaybeThrowInvalidPoint(double p) { | ||
| if (!S1Interval::IsValidPoint(p)) throw py::value_error("Invalid S1 point"); |
There was a problem hiding this comment.
Hmm, so we actually throw a C++ exception here. Is there a way to make this work without C++ exceptions, like returning an error-like type that then gets converted to an exception in Python?
There was a problem hiding this comment.
I'm pretty sure this is the recommended approach:
https://pybind11.readthedocs.io/en/stable/advanced/exceptions.html
I haven't found any other ways to make pybind raise a Python exception without throwing a C++ exception.
There was a problem hiding this comment.
Add a comment to ehe BUILD and/or README that exceptions need to be enabled.
There was a problem hiding this comment.
Done, added to README in the Invalid Values section
deustis
force-pushed
the
deustis/s1interval_bindings
branch
from
52fb522 to
3137a7d
Compare
src/python/s1interval_bindings.cc
Outdated
|
|
||
| void MaybeThrowInvalidPoint(double p) { | ||
| if (!S1Interval::IsValidPoint(p)) throw py::value_error("Invalid S1 point: " + std::to_string(p)); |
2 participants
Add pybind11 bindings for S1Interval.
Also add interface notes to the python README
(Part of a series of addressing #524)