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

anetAstrometry.py uses self.distortionContext, which does not exist

    Details

      Description

      James Mullaney reported the following bug on confluence:

      We're using the astrometry.net extension to perform astrometry. In v15 and v16 of the stack, I've noticed that meas_extensions_astrometryNet/python/lsst/meas/extensions/astrometryNet/anetAstrometry.py refers to self.distortionContext (see here). However, it seems that the distortionContext function was removed on the 4th of November, 2017 by this commit. I may be mistaken, but this seems to be a bug introduced by not catching all references to distortionContext when making that commit.

        Attachments

          Activity

          Hide
          rowen Russell Owen added a comment -

          My apologies. distortionContext had to go because the WCS in exposures now always has distortion (if we have a model for it) so it became unnecessary. But I missed that reference and the task is apparently not unit tested. A global search of our code base turns up no other instances. astrometry_net is barely supported so I made the minimum fix and did not add a unit test.

          Show
          rowen Russell Owen added a comment - My apologies. distortionContext had to go because the WCS in exposures now always has distortion (if we have a model for it) so it became unnecessary. But I missed that reference and the task is apparently not unit tested. A global search of our code base turns up no other instances. astrometry_net is barely supported so I made the minimum fix and did not add a unit test.
          Hide
          rowen Russell Owen added a comment - - edited

          The old distortionContext simply returned the exposure's BBox if the WCS already had distortion, which WCS always do, now. So I made the obvious fix: use the exposure's BBox.

          Also there was an instance of wcs.hasDistortion (in a log message) that needed removal, since that method no longer exists .

          Show
          rowen Russell Owen added a comment - - edited The old distortionContext simply returned the exposure's BBox if the WCS already had distortion, which WCS always do, now. So I made the obvious fix: use the exposure's BBox. Also there was an instance of wcs.hasDistortion (in a log message) that needed removal, since that method no longer exists .
          Hide
          rowen Russell Owen added a comment -

          I decided to add a basic unit test, after all. I used a simplified version of a test in meas_astrom that tests AstrometryTask. I also moved a module from meas_astrom to meas_extensions_astrometryNet.py because that is the only place it was used and it had a bug I needed to fix for the unit test.

          Show
          rowen Russell Owen added a comment - I decided to add a basic unit test, after all. I used a simplified version of a test in meas_astrom that tests AstrometryTask. I also moved a module from meas_astrom to meas_extensions_astrometryNet.py because that is the only place it was used and it had a bug I needed to fix for the unit test.
          Hide
          nlust Nate Lust added a comment -

          some minor things to consider, but changing them is up to you. Provided passes tests, merge away. Also I approved the one pull request I saw on github, but I did review changes in both repositories.

          Show
          nlust Nate Lust added a comment - some minor things to consider, but changing them is up to you. Provided passes tests, merge away. Also I approved the one pull request I saw on github, but I did review changes in both repositories.
          Hide
          rowen Russell Owen added a comment - - edited

          Thank you. Sorry about the missing pull request (I just made that one). I've made one of the requested changes and a modified version of the other and will merge after Jenkins runs the updated version.

          Show
          rowen Russell Owen added a comment - - edited Thank you. Sorry about the missing pull request (I just made that one). I've made one of the requested changes and a modified version of the other and will merge after Jenkins runs the updated version.

            People

            • Assignee:
              rowen Russell Owen
              Reporter:
              rowen Russell Owen
              Reviewers:
              Nate Lust
              Watchers:
              Nate Lust, Paul Price, Russell Owen
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Summary Panel