Conversation
|
@jmr, ready for you review. Not sure why the CI is failing |
|
@jmr, I believe I've addressed your comments! |
jmr
left a comment
jmr
left a comment
There was a problem hiding this comment.
Sorry, I noticed a few more things.
|
|
||
| def test_repr(self): | ||
| a = s2.S1Angle.from_degrees(45.0) | ||
| self.assertEqual(repr(a), "S1Angle(45.0000000)") |
There was a problem hiding this comment.
hmm. add assertEqual(a, S1Angle(45.0000000)) here. I don't think we have a float constructor. Use from_degrees and ideally control the number of trailing 0s.
There was a problem hiding this comment.
Not quite sure if this addresses your comment, but added test variations that show the input precision doesn't affect the output precision (since that's controlled by the C++ implementation)
|
|
||
| def test_e5(self): | ||
| a = s2.S1Angle.from_degrees(45.0) | ||
| self.assertEqual(a.e5(), 4500000) |
There was a problem hiding this comment.
I think all these eN / radians / degrees should be properties.
There was a problem hiding this comment.
radians, degrees, e5, e6, e7 are now properties. Updated the README property rule to cover simple accessors including trivial unit conversions.
| "Raises ValueError if out of range.") | ||
| .def("__abs__", &S1Angle::abs, | ||
| "Return the absolute value of the angle") | ||
| .def("normalized", &S1Angle::Normalized, |
There was a problem hiding this comment.
might be worthwhile to expose Normalize()
There was a problem hiding this comment.
Exposed both is_normalized() and normalize()
| bind_r2rect(m); | ||
| bind_s1interval(m); | ||
| bind_s2point(m); | ||
| bind_s1angle(m); |
There was a problem hiding this comment.
This says "dependency order", but eithe make it alphabetic if this has no deps, or comment what the dep is.
There was a problem hiding this comment.
Reorganized into dependency tranches with per-line comments (e.g. // Deps: s2point).
|
|
||
| void MaybeThrowNotNormalized(const S1Angle& angle) { | ||
| if (!angle.IsNormalized()) { | ||
| throw py::value_error("Angle " + std::to_string(angle.degrees()) + |
There was a problem hiding this comment.
We should avoid std::to_string since it uses locale. (I missed this in a previous PR.) Use absl::StrCat here instead.
There was a problem hiding this comment.
Done — switched to absl::StrCat in both s1angle_bindings.cc and s1interval_bindings.cc.
| "Raises ValueError if out of range.") | ||
| .def("__abs__", &S1Angle::abs, | ||
| "Return the absolute value of the angle") | ||
| .def("normalized", &S1Angle::Normalized, |
|
|
||
| def test_sin(self): | ||
| a = s2.S1Angle.from_degrees(90.0) | ||
| self.assertAlmostEqual(s2.sin(a), 1.0) |
There was a problem hiding this comment.
Is this is going to be a problem when we have S1ChordAngle.sin?
Will s2.sin have type cases to handle both types?
Maybe better to go with a.sin(). Writing s2.sin(a) doesn't seem that natural anyway.
There was a problem hiding this comment.
Moved sin/cos/tan from module-level functions to instance methods (a.sin(), a.cos(), a.tan()). This avoids the S1ChordAngle collision and reads more naturally.
- Make radians/degrees/e5/e6/e7 properties instead of methods - Expose IsNormalized() and Normalize() - Replace std::to_string with absl::StrCat (locale safety) - Move sin/cos/tan from module-level functions to instance methods - Organize module.cc into dependency tranches with comments - Update README property rule
deustis
force-pushed
the
deustis/s1angle_bindings
branch
from
77385b2 to
34c27c7
Compare
|
@jmr, PTAL! |
2 participants
Add pybind11 bindings for S1Angle.
Also add S1Angle::IsNormalizedAngle() to s1angle.h for pre-validating
E5/E6/E7 conversions.
Supports downstream PRs for S2LatLng (deustis#2) and
S2CellId (deustis#3).
(Part of a series of addressing #524)