New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DM-32097: minor improvements to Angle and SpherePoint #38
Conversation
It's a shame that both exist, but I'm not up for the disruption of fixing it. The least we can do is provide conversions, as we already do between geom::SpherePoint and sphgeom::LonLat. The conversions in C++ are explicit because I didn't want to have to think about the potential for surprising behavior when that interacts with operator overloading. That's not a problem in Python, so there the conversion is implicit, and hence one can now pass geom::Angle to pybind11-wrapped sphgeom routines that expect sphgeom::Angle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple of small comments/suggestions.
if (std::isfinite(self.asDegrees())) { | ||
return py::str("Angle({:0.17g}, degrees)").format(self.asDegrees()); | ||
} else { | ||
return py::str("Angle(float('{}'), degrees)").format(self.asDegrees()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can't put these two together into one statement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just found the single-statement version harder to read.
tests/test_angle.py
Outdated
values. | ||
""" | ||
from lsst.geom import Angle, degrees # need symbols in scope for eval | ||
self.assertEqual(eval(repr(Angle(4.0, degrees))), Angle(4.0, degrees)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do wonder if it's worth using a number where the 17-digit precision actually matters here? e.g. 0.12345678901234567
The new implementation uses degrees instead of radians - for consistency with SpherePoint.__repr__, - and because str(Angle) does radians and it's nice to have an easy way to do both.
Having one symbol (SpherePoint) assumed to be imported as from lsst.geom import SpherePoint while the other (degrees) is assumed to be imported via from lsst import geom in order to eval() a single repr() doesn't make sense, especially when our code never uses the latter pattern anyway.
63f3272
to
d2156a9
Compare
This branch has passed Jenkins and has been fully reviewed, so I'm merging it in advance of the rest of the ticket - other changes are in the new lsst-dm image_cutout_backend package, which is not part of lsst_distrib. |
No description provided.