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

astrometry has a bug in retrieving TAN_PIXELS and it exposes the need for a better API

    Details

    • Type: Bug
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: afw, meas_astrom
    • Labels:
      None
    • Team:
      Alert Production

      Description

      meas_astrom astrometry.py has two errors in how it retrieves the TAN_PIXELS coordinate system:

      pixelsToTanPixels = exposureInfo.getDetector().getTransformMap().get(TAN_PIXELS)
      if pixelsToTanPixels:
          ...

      The first error is using a CameraSysPrefix instead of a CameraSys to specify the transform; the second error is in assuming get will return None if the system is not present. Unfortunately the correct code is ugly and suggests the need for a better API:

      detector = exposureInfo.getDetector()
      tanPixCamSys = detector.getCameraSys(TAN_PIXELS)
      if tanPixCamSys in detector.getTransformMap():
          pixelsToTanPixels = detector.getTransformMap().get(tanPixCamSys)
          ...

      TransformMap can only handle CameraSys and fixing that would be hard, so I propose instead to add two overloaded methods to Detector, both of which can accept a CameraSys or a CameraSysPrefix:

      • getTransform(sys)
      • hasTransform(sys)

      The resulting code will be:

      detector = exposureInfo.getDetector()
      if detector.hasTransform(TAN_PIXELS):
          pixelsToTanPixels = detector.getTransform(TAN_PIXELS)
          ...

        Attachments

          Activity

          Hide
          rowen Russell Owen added a comment -

          The work is on tickets/DM-2471 of afw and meas_astrom

          The afw changes consist of adding two member functions to Detector, both of which can take a CameraSys or a CameraSysPrefix:

          • hasTransform
          • getTransform
            I expanded tests/testDetector.py to test these new member functions.

          The meas_astrom changes consist of rewriting the code that creates the DistortedTanWcs if the necessary information is available. Unfortunately that is not yet tested (and was not before). I propose making such a test a new ticket in order to move this along (meanwhile Dominique can test it directly with his CFHT data), but I can postpone if need be.

          Show
          rowen Russell Owen added a comment - The work is on tickets/ DM-2471 of afw and meas_astrom The afw changes consist of adding two member functions to Detector, both of which can take a CameraSys or a CameraSysPrefix: hasTransform getTransform I expanded tests/testDetector.py to test these new member functions. The meas_astrom changes consist of rewriting the code that creates the DistortedTanWcs if the necessary information is available. Unfortunately that is not yet tested (and was not before). I propose making such a test a new ticket in order to move this along (meanwhile Dominique can test it directly with his CFHT data), but I can postpone if need be.
          Hide
          rowen Russell Owen added a comment - - edited

          Simon: please try again. I accidentally branched afw from another branch instead of master. I have corrected that mistake.

          I also added a new function getDistortedWcs to afw.image.utils and modified AstrometryTask's run method to call that. This simplifies the code and makes it more robust, since the new function is reusable and tested.

          Finally I reduced the default SIP order of the TAN-SIP fitter from 5 to 3, to improve robustness. The fitter can easily go unstable at higher orders.

          Show
          rowen Russell Owen added a comment - - edited Simon: please try again. I accidentally branched afw from another branch instead of master. I have corrected that mistake. I also added a new function getDistortedWcs to afw.image.utils and modified AstrometryTask's run method to call that. This simplifies the code and makes it more robust, since the new function is reusable and tested. Finally I reduced the default SIP order of the TAN-SIP fitter from 5 to 3, to improve robustness. The fitter can easily go unstable at higher orders.
          Hide
          ktl Kian-Tat Lim added a comment -

          Looking at the old branch, the only comment I have is that you should use "Does ... exist" rather than "Does ... exists".

          Show
          ktl Kian-Tat Lim added a comment - Looking at the old branch, the only comment I have is that you should use "Does ... exist" rather than "Does ... exists".
          Hide
          krughoff Simon Krughoff added a comment -

          As long as this passes a buildbot build, it looks good to me.

          Show
          krughoff Simon Krughoff added a comment - As long as this passes a buildbot build, it looks good to me.

            People

            • Assignee:
              rowen Russell Owen
              Reporter:
              rowen Russell Owen
              Reviewers:
              Simon Krughoff
              Watchers:
              Dominique Boutigny, Kian-Tat Lim, Russell Owen, Simon Krughoff
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Summary Panel