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

eliminate confusing config side-effects in CalibrateTask

    XMLWordPrintable

    Details

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

      Description

      CalibrateTask does some unexpected things differently if you configure it certain ways, because it perceives certain processing as only being necessary to feed other steps. In particular, if you disable astrometry and photometric calibration, it only runs measurement once, because it assumes the only purpose of the post-PSF measurement is to feed those algorithms. This (as well as poor test coverage) made it easy to break CalibrateTask in the case where those options are disabled a few branches back.

      After conferring with Simon and Andy, we think the best solution is to remove this sort of conditional processing from CalibrateTask, which should also make it much easier to read. Instead, we'll always do both the initial and final phase of measurement, even if one of those phases is not explicitly being used within CalibrateTask itself.

        Attachments

          Activity

          Hide
          pgee Perry Gee added a comment -

          Simon – could you review this, since I understand that the request came from you? Note that the final decision was to leave things alone, except to run both measurement steps regardless of other options. If that isn't what you had in mind, please send it back.

          Changes are in two packages:
          python/lsst/pipe/tasks/calibrate.py | 26 +++++++++++++-------------
          1 file changed, 13 insertions, 13 deletions

          python/lsst/obs/sdss/calibrate.py | 14 +++++++-------
          1 file changed, 7 insertions, 7 deletions

          Show
          pgee Perry Gee added a comment - Simon – could you review this, since I understand that the request came from you? Note that the final decision was to leave things alone, except to run both measurement steps regardless of other options. If that isn't what you had in mind, please send it back. Changes are in two packages: python/lsst/pipe/tasks/calibrate.py | 26 +++++++++++++------------- 1 file changed, 13 insertions , 13 deletions python/lsst/obs/sdss/calibrate.py | 14 +++++++------- 1 file changed, 7 insertions , 7 deletions
          Hide
          krughoff Simon Krughoff added a comment -

          Perry, I'll get right on this.

          Show
          krughoff Simon Krughoff added a comment - Perry, I'll get right on this.
          Hide
          krughoff Simon Krughoff added a comment -

          Review comments:
          python/lsst/obs/sdss/calibrate.py
          line 315: It makes me nervous that the cause of this issue is conditionally running measurement and there is still a conditional on running measurement. Should I worry?

          Other than that, looks fine to me.

          Show
          krughoff Simon Krughoff added a comment - Review comments: python/lsst/obs/sdss/calibrate.py line 315: It makes me nervous that the cause of this issue is conditionally running measurement and there is still a conditional on running measurement. Should I worry? Other than that, looks fine to me.
          Hide
          pgee Perry Gee added a comment -

          I was attempting to make this change with minimal side-effects, so I left the code as I found except for some rearranging of conditionals. There is one call to measurement.measure in the not doPsf case, and a second call to measurement.run in the doPsf case. They are the same, so measurement gets called in either case.

          Show
          pgee Perry Gee added a comment - I was attempting to make this change with minimal side-effects, so I left the code as I found except for some rearranging of conditionals. There is one call to measurement.measure in the not doPsf case, and a second call to measurement.run in the doPsf case. They are the same, so measurement gets called in either case.
          Hide
          krughoff Simon Krughoff added a comment -

          I see now. Thanks for the explanation. The doPsf case didn't show up in the diff. Looks fine to me.

          Show
          krughoff Simon Krughoff added a comment - I see now. Thanks for the explanation. The doPsf case didn't show up in the diff. Looks fine to me.

            People

            Assignee:
            pgee Perry Gee
            Reporter:
            jbosch Jim Bosch
            Reviewers:
            Simon Krughoff
            Watchers:
            Andrew Becker [X] (Inactive), Jim Bosch, Perry Gee, Simon Krughoff
            Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

              Dates

              Created:
              Updated:
              Resolved:

                Jenkins

                No builds found.