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

Add jointcal, skyCorr to forcedPhotCcd

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: meas_base, pipe_tasks
    • Labels:
    • Team:
      External
    • Urgent?:
      No

      Description

      forcedPhotCcd needs to be able to apply the jointcal/fgcm and skyCorr recalibrations. Because this matches logic elsewhere in the pipeline (MakeCoaddTempExpTask), this will involve some refactoring, and because it involves I/O, we need to keep Gen3 happy.

        Attachments

          Issue Links

            Activity

            price Paul Price created issue -
            price Paul Price made changes -
            Field Original Value New Value
            Link This issue relates to DM-17062 [ DM-17062 ]
            price Paul Price made changes -
            Status To Do [ 10001 ] In Progress [ 3 ]
            Hide
            price Paul Price added a comment -

            Jim Bosch or Nate Lust: would you please review these changes?

            Show
            price Paul Price added a comment - Jim Bosch or Nate Lust : would you please review these changes?
            price Paul Price made changes -
            Reviewers Jim Bosch, Nate Lust [ jbosch, nlust ]
            Status In Progress [ 3 ] In Review [ 10004 ]
            Hide
            jbosch Jim Bosch added a comment - - edited

            Lots of little comments on the PRs; thanks for cleaning up various little messes along the way.

            But overall, I have to say I'm really torn about this:

            • I'm afraid I don't think the Task adds much value; there's just so little shared logic (much of which really belongs in PhotoCalib), and eventually there will be even less. It would still be a small positive addition, but...
            • There's a ton of config churn, and while I think our rules and precedent don't require an RFC for that (some of the configs you are deprecating here were added a few weeks ago on DM-21308, and that was not accompanied by an RFC), I think in the future, they probably should: config changes are at least as visible as code interface changes, and often much more.

            That said, adding the testing to the ci_hscs and of course adding recalibration to ForcedPhotCcd is useful new functionality that we absolutely want.

            So, I'd feel a lot more comfortable with this if people whose daily work is more affected by config deprecations than mine (e.g. Yusra AlSayyad, Lauren MacArthur) chimed in on this. And I'd be interested to get your opinion on how tough it would be to take the config options out of the subtask and put them back in the parent task(s), presumably by passing them down as bools (sorry!).

            Show
            jbosch Jim Bosch added a comment - - edited Lots of little comments on the PRs; thanks for cleaning up various little messes along the way. But overall, I have to say I'm really torn about this: I'm afraid I don't think the Task adds much value; there's just so little shared logic (much of which really belongs in PhotoCalib), and eventually there will be even less. It would still be a small positive addition, but... There's a ton of config churn, and while I think our rules and precedent don't require an RFC for that (some of the configs you are deprecating here were added a few weeks ago on DM-21308 , and that was not accompanied by an RFC), I think in the future, they probably should: config changes are at least as visible as code interface changes, and often much more. That said, adding the testing to the ci_hscs and of course adding recalibration to ForcedPhotCcd is useful new functionality that we absolutely want. So, I'd feel a lot more comfortable with this if people whose daily work is more affected by config deprecations than mine (e.g. Yusra AlSayyad , Lauren MacArthur ) chimed in on this. And I'd be interested to get your opinion on how tough it would be to take the config options out of the subtask and put them back in the parent task(s), presumably by passing them down as bools (sorry!).
            jbosch Jim Bosch made changes -
            Status In Review [ 10004 ] Reviewed [ 10101 ]
            Hide
            price Paul Price added a comment -

            After discussing with Jim, the plan is to strip the RecalibrateExposureTask down to a free function that applies the recalibrations, and leave the machinery for the Gen3 side for the future (DM-17062).

            I'll preserve the current state on a separate branch, in case anyone thinks it's worthy of resurrection at some future point. I'll keep the miscellaneous fixes if I can.

            Show
            price Paul Price added a comment - After discussing with Jim, the plan is to strip the RecalibrateExposureTask down to a free function that applies the recalibrations, and leave the machinery for the Gen3 side for the future ( DM-17062 ). I'll preserve the current state on a separate branch, in case anyone thinks it's worthy of resurrection at some future point. I'll keep the miscellaneous fixes if I can.
            Hide
            price Paul Price added a comment -

            Preserved the current state of meas_base, pipe_tasks, ci_hsc_gen2 and ci_hsc_gen3 on branch tickets/DM-23352-abort.

            Show
            price Paul Price added a comment - Preserved the current state of meas_base, pipe_tasks, ci_hsc_gen2 and ci_hsc_gen3 on branch tickets/DM-23352-abort .
            Hide
            lauren Lauren MacArthur added a comment -

            So it looks like the config-churning changes have been aborted. While I am always willing to adapt to major/backward-incompatible changes when there’s significant/unavoidable immediate &/or forward-looking gain, that didn’t appear to obviously be the case here. Having just spent an entire day towards adapting the pipe_analysis scripts to the new “external” calibration config names of DM-21308, I’ll just say “phew”

            Show
            lauren Lauren MacArthur added a comment - So it looks like the config-churning changes have been aborted. While I am always willing to adapt to major/backward-incompatible changes when there’s significant/unavoidable immediate &/or forward-looking gain, that didn’t appear to obviously be the case here. Having just spent an entire day towards adapting the pipe_analysis scripts to the new “external” calibration config names of DM-21308 , I’ll just say “phew”
            Hide
            price Paul Price added a comment -

            OK, I think this is ready for another look. Packages of interest:

            I'm leaving off packages that you've already signed off on and I didn't change (e.g., pex_config, daf_butler; obs_lsst goes away).

            Show
            price Paul Price added a comment - OK, I think this is ready for another look. Packages of interest: meas_base pipe_tasks ci_hsc_gen2 ci_hsc_gen3 obs_base obs_subaru I'm leaving off packages that you've already signed off on and I didn't change (e.g., pex_config, daf_butler; obs_lsst goes away).
            Hide
            jbosch Jim Bosch added a comment -

            Looks good. I'm sorry it's been such an ordeal to get here. I haven't tried to follow the logic of whether we should worry about MakeWarpTask trying and failing to apply recalibrations in ci_hsc_gen3, but Jenkins will take care of that.

            Show
            jbosch Jim Bosch added a comment - Looks good. I'm sorry it's been such an ordeal to get here. I haven't tried to follow the logic of whether we should worry about MakeWarpTask trying and failing to apply recalibrations in ci_hsc_gen3, but Jenkins will take care of that.
            Hide
            price Paul Price added a comment -

            Jenkins is green. I'll be merging soon.

            Show
            price Paul Price added a comment - Jenkins is green . I'll be merging soon.
            price Paul Price made changes -
            Resolution Done [ 10000 ]
            Status Reviewed [ 10101 ] Done [ 10002 ]
            Hide
            price Paul Price added a comment -

            Merged to master. Thanks to everyone for their patience, and especially to Jim and Nate for bearing with my many questions about Gen3.

            Show
            price Paul Price added a comment - Merged to master. Thanks to everyone for their patience, and especially to Jim and Nate for bearing with my many questions about Gen3.

              People

              • Assignee:
                price Paul Price
                Reporter:
                price Paul Price
                Reviewers:
                Jim Bosch, Nate Lust
                Watchers:
                Eli Rykoff, Jim Bosch, Lauren MacArthur, Nate Lust, Paul Price
              • Votes:
                0 Vote for this issue
                Watchers:
                5 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel