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
Add crude ERA from LST method and a test for it. tickets/DM-8052 #10
Conversation
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.
One minor suggestion
LST = 90*degrees | ||
Longitude = 50*degrees | ||
era = self.makeRawVisitInfo.eraFromLstAndLongitude(LST, Longitude) | ||
self.assertEqual(era, LST-Longitude) |
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'm uneasy about testing exact equality here, since these are floats. Wouldn't assertAnglesAlmostEqual (or whatever it's called) be safer?
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.
Good point. It's assertAnglesNearlyEqual
now.
@@ -241,6 +241,15 @@ def popMjdDate(self, md, key, timesys=None): | |||
return BadDate | |||
|
|||
@staticmethod | |||
def eraFromLstAndLongitude(lst, longitude): |
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.
Please document your longitude sign convention.
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.
Also I believe this takes and returns lsst.afw.geom.Angle
objects, NOT angles in degrees. Please fix the documentation accordingly. Please consider testing at least one argument to make sure it is an Angle and not a float (since Angle - float is an error, you just need to test one, but you could certainly test both for clarity).
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've documented longitude and added a test to check that it raises on float-Angle.
22e64d4
to
4ae8f78
Compare
No description provided.