DM-12473: Add getParallacticAngle() to visitInfo #297

isullivan · 2017-12-15T23:15:58Z

Adds a simple function to VisitInfo, that calculates the parallactic angle at the boresight of the exposure.

r-owen

Please add a unit test and more documentation as to what the returned angle measures. I had a few other small suggestions as well.

r-owen · 2017-12-18T21:42:53Z

include/lsst/afw/image/VisitInfo.h

@@ -174,6 +174,9 @@ class VisitInfo : public table::io::PersistableFacade<VisitInfo>, public table::
    // get hour angle at the boresight
    geom::Angle getBoresightHourAngle() const;

+    // get parallactic angle at the boresight


Please expand this comment so define the returned angle -- it is the angle from what to what?

r-owen · 2017-12-18T21:43:54Z

src/image/VisitInfo.cc

@@ -414,6 +414,15 @@ geom::Angle VisitInfo::getLocalEra() const { return getEra() + getObservatory().

 geom::Angle VisitInfo::getBoresightHourAngle() const { return getLocalEra() - getBoresightRaDec()[0]; }

+geom::Angle VisitInfo::getBoresightParAngle() const {
+  double _parallactic_y, _parallactic_x, result;
+  _parallactic_y = sin(getBoresightHourAngle());


Please make the conversion to radians explicit, e.g. getBoresightHourAngle.asRadians()

r-owen · 2017-12-18T21:44:37Z

src/image/VisitInfo.cc

@@ -425,6 +434,7 @@ std::ostream& operator<<(std::ostream& os, VisitInfo const& visitInfo) {
    os << "boresightRaDec=" << visitInfo.getBoresightRaDec() << ", ";
    os << "boresightAzAlt=" << visitInfo.getBoresightAzAlt() << ", ";
    os << "boresightAirmass=" << visitInfo.getBoresightAirmass() << ", ";
+    os << "boresightParAngle=" << visitInfo.getBoresightParAngle() << ", ";


I suggest omitting this because it is not a constructor argument and the representation is intended to be an executable representation in Python

isullivan · 2017-12-19T19:51:50Z

I have added three new unit tests and edited the comments to define parallactic angle more precisely.

r-owen

Thanks for the unit test and improved docs. I'm afraid I still want more of each -- more doc information so the sign is clear, and a non-trivial test (with corrected doc string) so the math is thoroughly tested.

r-owen · 2017-12-19T20:10:25Z

include/lsst/afw/image/VisitInfo.h

@@ -174,6 +174,10 @@ class VisitInfo : public table::io::PersistableFacade<VisitInfo>, public table::
    // get hour angle at the boresight
    geom::Angle getBoresightHourAngle() const;

+    // get parallactic angle at the boresight
+    // Equal to the angle measured at zenith between the North celestial pole and the boresight.


This still does not specify the sign of the angle. Please add more words or reword. Here is one long, if clumsy, possibility: "This is the angular separation between two great circle arcs that meet at the zenith: the meridian and the arc from the zenith to the target. The angle is 0 if the target is on the meridian north of the zenith, positive if the object is east of the meridian, and pi if the object is on the meridian south of the zenith."

Also, use a /** style comment block when the doc is more than one line (as it now is, and justifiably so).

r-owen · 2017-12-19T20:12:08Z

tests/test_visitInfo.py

+            self.assertAnglesAlmostEqual(visitInfo.getBoresightParAngle(), parAngle)
+
+    def test_parallacticAngleNorthMeridian(self):
+        """An observation on the Meridian in the Northern hemisphere has a parallactic angle of pi radians."""


In the office you said it will be pi if the target is south of the telescope, 0 if north of the telescope on the meridian. I think that is clearer than "in the Norther hemisphere", especially since the hemisphere should not matter.

Also please add a test for positive or negative not on the meridian. The two tests you have now will pass regardless of the sign of the parallactic angle! I realize it may be a bit trickier to predict the correct angle, but I think it is well worth testing at least one non-trivial case, since trivial cases can all too easily hide math errors.

Also I suggest you simplify this test by only setting the VisitInfo fields that matter. VisitInfo can be constructed using keyword arguments and any of them can be omitted.

Good points, you are correct. I will update the two meridian tests to be north and south of zenith, not in the northern or southern hemisphere.

For the test of the sign of the parallactic angle, doesn't the first test test_parallacticAngle catch that? There it checks the results for the coordinates of data1 and data2 against precomputed values.

Yes, test_parallacticAngle is exactly what I was asking for. My apologies for missing that.

Shouldn't have underscores in Science Pipelines code.

Okay. I was copying the style of the test immediately before the ones I added, but I suppose that one was an exception.

isullivan · 2017-12-19T21:02:06Z

Please take another look. I have revised the documentation and the tests, and fixed a discrepancy in the definition while working it over. Please also check if the test test_parallacticAngle satisfies your request for a non-trivial test.
I will plan to rebase to clean up all of the commits before merging.

r-owen

This looks great. Thanks for the improvements.

r-owen · 2017-12-19T20:39:34Z

tests/test_visitInfo.py

+            self.assertAnglesAlmostEqual(visitInfo.getBoresightParAngle(), parAngle)
+
+    def test_parallacticAngleNorthMeridian(self):
+        """An observation on the Meridian in the Northern hemisphere has a parallactic angle of pi radians."""


Yes, test_parallacticAngle is exactly what I was asking for. My apologies for missing that.

r-owen · 2017-12-20T00:18:53Z

tests/test_visitInfo.py

@@ -48,7 +50,7 @@ def propertySetFromDict(keyValDict):
    return metadata


-class VisitInfoTestCase(unittest.TestCase):
+class VisitInfoTestCase(lsst.utils.tests.TestCase):


Thank you for this change

r-owen · 2017-12-20T00:49:40Z

Regarding @PaulPrice's comment about the test case names: I think we usually prefer to keep things consistent within a file. You could update all the names, I suppose.

r-owen requested changes Dec 18, 2017

View reviewed changes

r-owen requested changes Dec 19, 2017

View reviewed changes

r-owen approved these changes Dec 20, 2017

View reviewed changes

r-owen changed the title ~~Add function to VisitInfo to return parallactic angle.~~ DM-12473: Add getParallacticAngle() to visitInfo Dec 20, 2017

Add function to VisitInfo to return parallactic angle.

2c2024f

isullivan force-pushed the tickets/DM-12473 branch from 20da5e9 to 2c2024f Compare December 20, 2017 00:35

isullivan merged commit bdf48f8 into master Dec 20, 2017

ktlim deleted the tickets/DM-12473 branch August 25, 2018 06:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-12473: Add getParallacticAngle() to visitInfo #297

DM-12473: Add getParallacticAngle() to visitInfo #297

isullivan commented Dec 15, 2017

r-owen left a comment

r-owen Dec 18, 2017

r-owen Dec 18, 2017

r-owen Dec 18, 2017 •

edited

isullivan commented Dec 19, 2017

r-owen left a comment

r-owen Dec 19, 2017

r-owen Dec 19, 2017

isullivan Dec 19, 2017

r-owen Dec 19, 2017

PaulPrice Dec 20, 2017

isullivan Dec 20, 2017

isullivan commented Dec 19, 2017

r-owen left a comment

r-owen Dec 19, 2017

r-owen Dec 20, 2017 •

edited

r-owen commented Dec 20, 2017

DM-12473: Add getParallacticAngle() to visitInfo #297

DM-12473: Add getParallacticAngle() to visitInfo #297

Conversation

isullivan commented Dec 15, 2017

r-owen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r-owen Dec 18, 2017 • edited

Choose a reason for hiding this comment

isullivan commented Dec 19, 2017

r-owen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isullivan commented Dec 19, 2017

r-owen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r-owen Dec 20, 2017 • edited

Choose a reason for hiding this comment

r-owen commented Dec 20, 2017

r-owen Dec 18, 2017 •

edited

r-owen Dec 20, 2017 •

edited