Skip to content
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-11162: Replace all use of Coord and subclasses with SpherePoint #61

Merged
merged 2 commits into from Mar 23, 2018

Conversation

r-owen
Copy link
Contributor

@r-owen r-owen commented Mar 16, 2018

No description provided.

@@ -68,17 +67,17 @@ def testRaw(self):
self.assertEqual(raw.getDetector().getName(), "R:0,3 S:0,1")
origin = raw.getWcs().getSkyOrigin()
self.assertAlmostEqual(
origin.getLongitude().asDegrees(), 0.005865, 6)
origin.getLongitude().asDegrees(), 0.0058520, 6)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this value changed? Same question below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The basic problem is that old phosim data had incorrect WCS information in the FITS headers: the date of equinox was not J2000 but was, instead, the date of observation. The old technique for fixing this isn't easily applicable with the new code, and was slightly incorrect. The new technique is more accurate. (Not that the difference between the techniques really matters, since the WCS from phosim isn't meant to be highly accurate in the first place. The important thing is to fix the gross error).

exposure.setWcs(wcs)

return exposure
# CRVAL is FK5 at date of observation
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm trying to understand the changes you made here. Before, the code was converting the wcs from an an Fk5 to Icrs. The new code only changes the metadata. Is this sufficient?

Copy link
Contributor Author

@r-owen r-owen Mar 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is a detail of the old technique (mentioned above) that wasn't quite correct. The FITS metadata claimed that CRVAL was ICRS but it was really FK5 with equinox = date of observation. The old code attempted to fix the WCS after it was normalized to ICRS, but the fix was only approximate. The simpler solution (new code) is to fix the FITS metadata before it is used to create the WCS.

Copy link
Contributor

@rearmstr rearmstr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

@r-owen r-owen merged commit 6e0b543 into master Mar 23, 2018
@ktlim ktlim deleted the tickets/DM-11162 branch August 25, 2018 05:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants