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

Use CoaddPsfs from all warps in getTemplate task

    XMLWordPrintable

    Details

    • Story Points:
      12
    • Sprint:
      AP S21-6 (May), AP F21-1 (June), AP F21-2 (July), AP F21-3 (August)
    • Team:
      Data Release Production
    • Urgent?:
      No

      Description

      Currently GetCoaddAsTemplateTask sets the PSF of the template with that of the first warp that it adds in. Bob Armstrong improved this on https://github.com/LSSTDESC/dia_pipe/blob/master/python/lsst/dia/pipe/getMultiTractTemplate.py. However, he reports an edge case where it fails trying to compute the average position of the composite PSF near edges because the average position can be in a region of no data.

      Bob Armstrong will ask Bruno for tract/patch that failed as a how-to-reproduce.

      This ticket covers ensuring that the PSF is valid across the whole template

        Attachments

          Issue Links

            Activity

            Hide
            yusra Yusra AlSayyad added a comment -

            Update: Data ID (visit=30500, ccd=54) triggers the “Could not find a valid average position for CoaddPsf”
            30500_54_templateGeom_annotated.pdf
            full visit/tract relationship: https://lsst.ncsa.illinois.edu/~yusra/download/30500.png

            Show
            yusra Yusra AlSayyad added a comment - Update: Data ID (visit=30500, ccd=54) triggers the “Could not find a valid average position for CoaddPsf” 30500_54_templateGeom_annotated.pdf full visit/tract relationship: https://lsst.ncsa.illinois.edu/~yusra/download/30500.png
            Hide
            yusra Yusra AlSayyad added a comment -

            John would you be up for reviewing this?

            I stepped boldly in to the future of gen3 and started by splitting the task that makes the template from the task that uses the template, and made fresh gen3-only tasks for both.

            This gets tested in both ci_imsim and ci_hsc_gen3.

            Jenkins run going now.

            Show
            yusra Yusra AlSayyad added a comment - John would you be up for reviewing this? I stepped boldly in to the future of gen3 and started by splitting the task that makes the template from the task that uses the template, and made fresh gen3-only tasks for both. This gets tested in both ci_imsim and ci_hsc_gen3. Jenkins run going now.
            Hide
            sullivan Ian Sullivan added a comment -

            I've reviewed the three pull requests, and the new and changed code looks fine. My broader concern is that this new task will replace getTemplate for DRP and AP large-scale processing runs, but does not share any of the code. I suspect this means that developers will also switch to using the new task for the rest of our CI and development work, and since getTemplate does not have proper unit tests it could easily bitrot. Once Gen 2 goes away, I don't see an advantage in maintaining both, since the new task can handle single as well as multi-tract templates. From that perspective, you might want to consider the more drastic change of replacing the Gen 3 version of getTemplate with your new code, rather than making it a new task.

            I also looked over the new code from the perspective of compatibility with DCR coadds, and it looks like it would be very easy to add that support.

            Show
            sullivan Ian Sullivan added a comment - I've reviewed the three pull requests, and the new and changed code looks fine. My broader concern is that this new task will replace getTemplate for DRP and AP large-scale processing runs, but does not share any of the code. I suspect this means that developers will also switch to using the new task for the rest of our CI and development work, and since getTemplate does not have proper unit tests it could easily bitrot. Once Gen 2 goes away, I don't see an advantage in maintaining both, since the new task can handle single as well as multi-tract templates. From that perspective, you might want to consider the more drastic change of replacing the Gen 3 version of getTemplate with your new code, rather than making it a new task. I also looked over the new code from the perspective of compatibility with DCR coadds, and it looks like it would be very easy to add that support.
            Hide
            yusra Yusra AlSayyad added a comment -

            These getTemplate tasks are aggregation steps that are 100% plumbing (no algorithms) and thus very tied to the middleware version. Most of the Gen2 GetCoaddAsTemplateTask code is finding which patches overlap the detector which is already done in the butler in gen 3.

            My wish is to eventually delete GetCoaddAsTemplateTask. We could do this as soon as all the conditions have been met:

            • GetMultiTractTemplateTask is fully commissioned on our all our pipelines and precursor datasets
            • Date > Jan 1 2022
            • We have a GetDCRCoaddAsTemplateTask (if that requires different dimensions).

            IMO, It's a feature rather than a bug they share no code.
            What they should share, however, is an API.
            I don't want to copy the GetCoaddAsTemplateTask API (there's a method called runQuantum that isn't a real runQuantum that ImageDifferenceTask calls as a subtask, which should never have happened). I can open a ticket to edit the original ImageDifferenceTask to be able to use the this new Gen3-only GetMultiTract task, but the fake runQuantum in GetCoaddAsTemplateTask is going away.

            Show
            yusra Yusra AlSayyad added a comment - These getTemplate tasks are aggregation steps that are 100% plumbing (no algorithms) and thus very tied to the middleware version. Most of the Gen2 GetCoaddAsTemplateTask code is finding which patches overlap the detector which is already done in the butler in gen 3. My wish is to eventually delete GetCoaddAsTemplateTask . We could do this as soon as all the conditions have been met: GetMultiTractTemplateTask is fully commissioned on our all our pipelines and precursor datasets Date > Jan 1 2022 We have a GetDCRCoaddAsTemplateTask (if that requires different dimensions). IMO, It's a feature rather than a bug they share no code. What they should share, however, is an API. I don't want to copy the GetCoaddAsTemplateTask API (there's a method called runQuantum that isn't a real runQuantum that ImageDifferenceTask calls as a subtask, which should never have happened). I can open a ticket to edit the original ImageDifferenceTask to be able to use the this new Gen3-only GetMultiTract task, but the fake runQuantum in GetCoaddAsTemplateTask is going away.
            Hide
            yusra Yusra AlSayyad added a comment -

            Oh and this passed Jenkins and I think is ready to be merged.

            Show
            yusra Yusra AlSayyad added a comment - Oh and this passed Jenkins and I think is ready to be merged.

              People

              Assignee:
              yusra Yusra AlSayyad
              Reporter:
              yusra Yusra AlSayyad
              Reviewers:
              Ian Sullivan
              Watchers:
              Bob Armstrong, Ian Sullivan, John Parejko, Yusra AlSayyad
              Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  CI Builds

                  No builds found.