Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-2280

The TAN_PIXELS cameraGeom coordinate system should be with respect to the center of the focal plane

    Details

    • Type: Bug
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: afw

      Description

      The TAN_PIXELS cameraGeom coordinate system (the position on a detector if there is no optical distortion) is presently defined with respect to the center of the detector – i.e. a star at the center of the detector will have the same position in PIXELS and TAN_PIXELS coordinates. That is a mistake. TAN_PIXELS should be defined with respect to the center of the focal plane, since it then reflects the effects of having optical distortion or not.

      Fixing this will help meas_astrom match stars. The effects of not fixing it are making the matcher search farther for a fit. As long as we allow sufficient offset in the matcher config the current system will work, but it is not ideal.

        Attachments

          Issue Links

            Activity

            Hide
            rhl Robert Lupton added a comment -

            I don't understand why a decision like this can make a difference in the astrometric matching. We need to handle the distortion correctly; a coordinate system like TAN_PIXELS may or may not make this easier or harder but the code has to be handling the full distortion mapping sky coordinates to positions in whatever coordinate system we're using.

            This isn't to say that this ticket shouldn't be addressed, but it shouldn't be a blocker for DM-6923

            Show
            rhl Robert Lupton added a comment - I don't understand why a decision like this can make a difference in the astrometric matching. We need to handle the distortion correctly; a coordinate system like TAN_PIXELS may or may not make this easier or harder but the code has to be handling the full distortion mapping sky coordinates to positions in whatever coordinate system we're using. This isn't to say that this ticket shouldn't be addressed, but it shouldn't be a blocker for DM-6923
            Hide
            rowen Russell Owen added a comment -

            This bug (DM-2280) is causing DM-6923 because of the way we represent WCS in post-ISR exposures. See my comment on DM-6923 for the technical explanation. Once we have a better model for WCS I suspect DM-6923 problem would have other solutions, but I still think fixing this issue is the right thing to do, and doing so will fix DM-6923.

            Show
            rowen Russell Owen added a comment - This bug ( DM-2280 ) is causing DM-6923 because of the way we represent WCS in post-ISR exposures. See my comment on DM-6923 for the technical explanation. Once we have a better model for WCS I suspect DM-6923 problem would have other solutions, but I still think fixing this issue is the right thing to do, and doing so will fix DM-6923 .
            Hide
            rowen Russell Owen added a comment -

            Changes are all in afw and are fairly minor:

            • Rewrote makePixelToTanPixel.py. The new version is significantly simpler than the old version.
            • Modified, expanded and modernized the unit test for makePixelToTanPixel
            • Updated cameraGeom.dox's description of the TAN_PIXELS camera coordinate system

            I have tested the code as follows:

            • Jenkins CI
            • Run validate_drp's examples on CFHT and DECam data; there were no significant changes between the results for master and this branch. This is not surprising, since at least in the case of CHFT the CCDs run by the test are all near the center of the focal plane.
            • Tweaked validate_drp's quick CFHT test to run on CFHT CCD 0, which is a corner CCD. This ticket branch gives somewhat better astrometry than master.

            I won't attempt to merge this until RFC-200 is approved.

            Show
            rowen Russell Owen added a comment - Changes are all in afw and are fairly minor: Rewrote makePixelToTanPixel.py. The new version is significantly simpler than the old version. Modified, expanded and modernized the unit test for makePixelToTanPixel Updated cameraGeom.dox's description of the TAN_PIXELS camera coordinate system I have tested the code as follows: Jenkins CI Run validate_drp's examples on CFHT and DECam data; there were no significant changes between the results for master and this branch. This is not surprising, since at least in the case of CHFT the CCDs run by the test are all near the center of the focal plane. Tweaked validate_drp's quick CFHT test to run on CFHT CCD 0, which is a corner CCD. This ticket branch gives somewhat better astrometry than master. I won't attempt to merge this until RFC-200 is approved.
            Hide
            rearmstr Bob Armstrong added a comment -

            Sorry for the delay on the review. It took more time than I thought to understand what was going on. I only had a few minor comments/questions on the pull request, but other than that it looks good. The new code is definitely easier to understand!

            Show
            rearmstr Bob Armstrong added a comment - Sorry for the delay on the review. It took more time than I thought to understand what was going on. I only had a few minor comments/questions on the pull request, but other than that it looks good. The new code is definitely easier to understand!
            Hide
            rowen Russell Owen added a comment -

            Thank you for the helpful review. I removed the plateScale argument from makePixelToTanPixel and, by extension, makeDetector. I updated all calling code, which included small changes to ip_isr and obs_test.

            Show
            rowen Russell Owen added a comment - Thank you for the helpful review. I removed the plateScale argument from makePixelToTanPixel and, by extension, makeDetector. I updated all calling code, which included small changes to ip_isr and obs_test.

              People

              • Assignee:
                rowen Russell Owen
                Reporter:
                rowen Russell Owen
                Reviewers:
                Bob Armstrong
                Watchers:
                Bob Armstrong, Paul Price, Robert Lupton, Russell Owen, Simon Krughoff
              • Votes:
                0 Vote for this issue
                Watchers:
                5 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel