pybind: Add S1Interval bindings (#524)

[deustis]

Add pybind11 bindings for S1Interval.

Also add interface notes to the python README

(Part of a series of addressing #524)

[deustis]

@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.

[jmr]

I'm not sure about this. Definitely needs a comment.

[deustis]

Agree, let's align on this thread and then I'll update this:
#534 (comment)

[jmr]

Hmm. Why? Isn't this an ODR violation waiting to happen?

[deustis]

The challenge is that the C++ implementation uses debug assertions which is perhaps the source of the "ODR violation". That leaves us two choices for the Python interface: adhere to the dbg behavior or the opt behavior.

The current PR does the latter, meaning anywhere the opt C++ code accepts invalid inputs the Python does the same (as well as exposing an is_valid if that is available in C++).

Alternatively, we could follow the dbg behavior where the user would expect the Python code to raise exceptions. The challenge is that the ABSL debug checks abort the process and aren't "catchable" in the Python bindings. So we would need to either change the C++ assertions, or reproduce the validation in the Pybind implementation itself.

From the C++ perspective, would you say that the debug assertions are enough of a deterrent that we can say the opt behavior is the de-facto interface?

In that case I agree we should probably lean into the cbg behavior. We might also consider switching from ABSL_DCHECK to ABSL_CHECK in the C++ code? I'm guessing we can't make larger interface changes to the C++ code such as raising exceptions or returning Status messages.

So that leaves us with trying to reproduce the validity checks in the Python bindings themselves. This is doable but it would be non-trivial For example, for S1Interval we would to refactor this validity function to be able to call it before calling the various C++ function that would fail with assertions:
https://github.com/google/s2geometry/blob/master/src/s2/s1interval.h#L247

inline bool S1Interval::is_valid() const {
  return (std::fabs(lo()) <= M_PI && std::fabs(hi()) <= M_PI &&
          !(lo() == -M_PI && hi() != M_PI) &&
          !(hi() == -M_PI && lo() != M_PI));
}

[jmr]

That leaves us two choices for the Python interface: adhere to the dbg behavior or the opt behavior.

Why can't the user choose by setting the compilation mode?

From the C++ perspective, would you say that the debug assertions are enough of a deterrent that we can say the opt behavior is the de-facto interface?

It's the same interface for dbg and opt. The interface is that there are preconditions and if they are violated, anything could happen. This is fine for C++ but is not very Pythonic.

Now I see that S1Interval is a bit odd in that it DCHECKs in the constructor rather just than when you try to use it. I think we could add S1Interval::FromBounds(lo, hi) -> optional<S1Interval> or StatusOr<S1Interval> (not sure how much the Status would add, but maybe good for consistency). We could also move the DCHECK out of the constructor.

The Python constructor could call this and throw ValueError, then we deal with only valid intervals. Maybe the setters need to go to preserve "always valid".

[deustis]

The interface is that there are preconditions and if they are violated, anything could happen.

😆 -- IMHO, not so great for C++ either.

This is fine for C++ but is not very Pythonic.

Agreed! Let's go with exceptions for the Python interface then? I'd also rather avoid differences in behavior depending on compilation mode.

Now I see that S1Interval is a bit odd in that it DCHECKs in the constructor

Thanks for the suggestions. I'll need to circle back to this next week but something along those lines should work.

[deustis]

@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.

[jmr]

Is there a reason you picked S1Interval? Seems obscure and low level.