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-16490: Update computeCcdOffsets() to use makeSkyWcs #17

Merged
merged 3 commits into from Sep 11, 2019

Conversation

erykoff
Copy link
Contributor

@erykoff erykoff commented Sep 6, 2019

This update deprecates the pixelScale configuration parameter which is no
longer necessary now that all the camera units are correct and makeSkyWcs works
properly for HSC. However, due to a bug in the data files in
testdata_jointcal, there is still an HSC-specific hack that can be fixed after
DM-17597 is complete.

This update deprecates the pixelScale configuration parameter which is no
longer necessary now that all the camera units are correct and makeSkyWcs works
properly for HSC.  However, due to a bug in the data files in
testdata_jointcal, there is still an HSC-specific hack that can be fixed after
DM-17597 is complete.
Copy link
Contributor

@parejkoj parejkoj left a comment

Choose a reason for hiding this comment

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

See comments: this is a good improvement overall.

@@ -214,11 +214,12 @@ class FgcmFitCycleConfig(pexConfig.Config):
dtype=float,
default=None,
)
# TODO: DM-16490 will make this unneccessary
# pixelScale is deprecated by DM-16490.
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment should go into the deprecated field: it's not necessary here.

boresight = geom.SpherePoint(180.0*geom.degrees, 0.0*geom.degrees)
# TODO: DM-17597 will update testdata_jointcal so that the test data
# does not have nan as the boresight angle for HSC data. For the
# time being, there is this ungainly hack.
Copy link
Contributor

Choose a reason for hiding this comment

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

Good TODO.

# along with this program. If not, see <https://www.gnu.org/licenses/>.
"""Test the fgcmcal computeCcdOffsets code with testdata_jointcal.

"""
Copy link
Contributor

Choose a reason for hiding this comment

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

extra line?


pixelsToFieldAngle = detector.getTransform(detector.makeCameraSys(PIXELS),
detector.makeCameraSys(FIELD_ANGLE))
wcs = afwGeom.makeSkyWcs(pixelsToFieldAngle, orientation, flipX, boresight)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason you can't use obs.base.utils.createInitialSkyWcs here instead? Or just use the stack-generated WCS instead of creating your own?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At first I didn't use obs.base.utils.createInitialSkyWcs() because of the workflow (extracting all the visit details and extracting stars before the actual run begins) didn't have access to the visitInfo. And this ccdOffset structure is used primarily for visualization and also some airmass corrections. But the fgcm code doesn't handle rotations properly yet (hence the TODO) so I'll have to revisit some of these assumptions.
But when responding to this comment I realized I could still create a fake visitInfo and call createInitialSkyWcs and save a couple lines. So this is what I've now done (and maybe this is what you had in mind). Furthermore, by creating the fake visitInfo I can still patch the rotation angle from testdata_jointcal.

self.assertGreater(ccdOffsets['DEC_SIZE'][i], 0.17)
self.assertLess(ccdOffsets['DEC_SIZE'][i], 0.20)
self.assertGreater(ccdOffsets['RA_SIZE'][i], 0.07)
self.assertLess(ccdOffsets['RA_SIZE'][i], 0.10)
Copy link
Contributor

Choose a reason for hiding this comment

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

Where did these sizes come from, and what are the units here? Why the 0.03 range in each case? Please document that in the code.

@erykoff erykoff merged commit 7277a70 into master Sep 11, 2019
@erykoff erykoff deleted the tickets/DM-16490 branch September 11, 2019 18:55
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