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

CalibrateTask has outdated, incorrect code for handling aperture corrections

    XMLWordPrintable

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: obs_cfht
    • Labels:
      None

      Description

      The CFHT-specific CalibrateTask tries to apply aperture correction once just after measuring it (which is too early) and again later, at the right time. The error probably has no effect on the final results, but it is confusing and needlessly divergent from the standard CalibrateTask. The required changes are small. I plan to test by running Dominique Boutigny's CFHT demo.

        Attachments

          Activity

          No builds found.
          rowen Russell Owen created issue -
          rowen Russell Owen made changes -
          Field Original Value New Value
          Epic Link DM-3543 [ 19632 ]
          rowen Russell Owen made changes -
          Description The CFHT-specific CalibrateTask has some outdated code for handling aperture corrections. The code is incorrect and divergent from the standard CalibrateTask. The required changes are small. I plan to test by running [~boutigny]'s CFHT demo. The CFHT-specific CalibrateTask tries to apply aperture correction once just after measuring it (which is too early) and again later, at the right time. The error probably has no effect on the final results, but it is confusing and needlessly divergent from the standard CalibrateTask. The required changes are small. I plan to test by running [~boutigny]'s CFHT demo.
          Hide
          rowen Russell Owen added a comment -

          I ran valid_cfht and all looks OK.

          Note that valid_cfht turned out to have some bit rot: the astrometry_net_data config lists files that don't exist, which upsets the new astrometry index caching code. I updated valid_cfht on /lsst8/boutigny/valid_cfht/astrometry_net_data/andConfig.py accordingly. The config files one level up will also want changing from "root" to "config", but it's safer to hold off on that for awhile.

          Show
          rowen Russell Owen added a comment - I ran valid_cfht and all looks OK. Note that valid_cfht turned out to have some bit rot: the astrometry_net_data config lists files that don't exist, which upsets the new astrometry index caching code. I updated valid_cfht on /lsst8/boutigny/valid_cfht/astrometry_net_data/andConfig.py accordingly. The config files one level up will also want changing from "root" to "config", but it's safer to hold off on that for awhile.
          Hide
          rowen Russell Owen added a comment -

          Ian: are you willing to have a look at this? If so, I suggest you compare CalibrateTask in pipe_tasks with the subclassed version in obs_cfht, focusing on areas where I made my changes (but anything else in the run method that looks wrong is fair game for questions or complaints).

          Show
          rowen Russell Owen added a comment - Ian: are you willing to have a look at this? If so, I suggest you compare CalibrateTask in pipe_tasks with the subclassed version in obs_cfht, focusing on areas where I made my changes (but anything else in the run method that looks wrong is fair game for questions or complaints).
          rowen Russell Owen made changes -
          Reviewers Ian Sullivan [ sullivan ]
          Status To Do [ 10001 ] In Review [ 10004 ]
          rowen Russell Owen made changes -
          Summary CalibrateTask has some outdated, incorrect for handling aperture corrections CalibrateTask has some outdated, incorrect code for handling aperture corrections
          rowen Russell Owen made changes -
          Summary CalibrateTask has some outdated, incorrect code for handling aperture corrections CalibrateTask has outdated, incorrect code for handling aperture corrections
          Hide
          sullivan Ian Sullivan added a comment -

          Looks okay to merge.

          Show
          sullivan Ian Sullivan added a comment - Looks okay to merge.
          sullivan Ian Sullivan made changes -
          Status In Review [ 10004 ] Reviewed [ 10101 ]
          Hide
          rowen Russell Owen added a comment -

          Merged to master

          Show
          rowen Russell Owen added a comment - Merged to master
          rowen Russell Owen made changes -
          Resolution Done [ 10000 ]
          Status Reviewed [ 10101 ] Done [ 10002 ]

            People

            Assignee:
            rowen Russell Owen
            Reporter:
            rowen Russell Owen
            Reviewers:
            Ian Sullivan
            Watchers:
            Dominique Boutigny, Ian Sullivan, Russell Owen, Simon Krughoff
            Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

              Dates

              Created:
              Updated:
              Resolved:

                Jenkins

                No builds found.