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

            No builds found.
            yusra Yusra AlSayyad created issue -
            yusra Yusra AlSayyad made changes -
            Field Original Value New Value
            Epic Link PREOPS-450 [ 463950 ]
            sullivan Ian Sullivan made changes -
            Assignee John Parejko [ parejkoj ]
            sullivan Ian Sullivan made changes -
            Sprint AP S21-5 (April) [ 1084 ]
            Team Alert Production [ 10300 ]
            Hide
            Parejkoj John Parejko added a comment -

            Following up on this: Bob Armstrong: do you have any information about the tract/patch that had failed?

            Show
            Parejkoj John Parejko added a comment - Following up on this: Bob Armstrong : do you have any information about the tract/patch that had failed?
            Hide
            rearmstr Bob Armstrong added a comment -

            I was able to find the patch that failed, but I don't think it will help you much. We were using our own processing of DC2 images. We regenerated the coadd template with a specific set of good seeing exposures in a limited time frame. Unless you reuse the same rerun the problem won't appear for the visit I found.

            Show
            rearmstr Bob Armstrong added a comment - I was able to find the patch that failed, but I don't think it will help you much. We were using our own processing of DC2 images. We regenerated the coadd template with a specific set of good seeing exposures in a limited time frame. Unless you reuse the same rerun the problem won't appear for the visit I found.
            Parejkoj John Parejko made changes -
            Description Currently getTemplate task sets the PSF of the template with that of the first warp that it adds in. [~rearmstr] 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.

            [~rearmstr] 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
            Currently {{GetCoaddAsTemplateTask}} sets the PSF of the template with that of the first warp that it adds in. [~rearmstr] 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.

            [~rearmstr] 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
            Hide
            rearmstr Bob Armstrong added a comment -

            I don't think it should be hard to reproduce. You need to find a visit that overlaps multiple tracts and also overlaps a boundary of the template you created.

            Show
            rearmstr Bob Armstrong added a comment - I don't think it should be hard to reproduce. You need to find a visit that overlaps multiple tracts and also overlaps a boundary of the template you created.
            yusra Yusra AlSayyad made changes -
            Remote Link This issue links to "Page (Confluence)" [ 28284 ]
            Hide
            Parejkoj John Parejko added a comment - - edited

            Following up from a conversation with Yusra, the best place to look for an example that triggers this is on the edge of tract HSC 9813; try band='i', detector=68, visit=30486 for an exposure on the edge. gen2 and gen3 were loading different tracts, so in gen2 there was no diffim generated for this image, while in gen3 there was but it triggered this bug. Here is an example pipetask command to try:

            pipetask run -b /repo/main -i u/yusra/w_2021_14_templates -o  u/parejkoj/DM-29310 -p $OBS_SUBARU_DIR/pipelines/DRP.yaml#imageDifference -d "instrument='HSC' AND skymap='hsc_rings_v1' AND tract=9813 AND band='i'  AND detector=68 AND visit=30486"

            See this image for the relevant tract/visit relationship here: https://lsst.ncsa.illinois.edu/~yusra/download/30486.png

            It's possible that the coadd that gets built in pipe_tasks nopytest_test_coadd.py would work to test this as well (there are no unittests of GetCoaddAsTemplateTask). It's worth checking the layout of the patches/tracts that get built there. The existing `testCoaddPsf` in that test suite might be worth looking at as a comparison. Given the call signature for getTemplate's `run`, we can probably mock up the input coordinates to trigger the problem, assuming the test geometry is right.

            Also, this is independent of the problem that GetCoaddAsTemplateTask only operates on one tract. Yusra is going to file a separate ticket to deal with that problem. That would require reworking `run` to accept multiple tracts, and would need e.g. tract 9812 to be built for the above detector+visit example that crosses tract boundaries.

            Show
            Parejkoj John Parejko added a comment - - edited Following up from a conversation with Yusra, the best place to look for an example that triggers this is on the edge of tract HSC 9813; try band='i', detector=68, visit=30486 for an exposure on the edge. gen2 and gen3 were loading different tracts, so in gen2 there was no diffim generated for this image, while in gen3 there was but it triggered this bug. Here is an example pipetask command to try: pipetask run -b /repo/main -i u/yusra/w_2021_14_templates -o u/parejkoj/DM-29310 -p $OBS_SUBARU_DIR/pipelines/DRP.yaml#imageDifference -d "instrument='HSC' AND skymap='hsc_rings_v1' AND tract=9813 AND band='i' AND detector=68 AND visit=30486" See this image for the relevant tract/visit relationship here: https://lsst.ncsa.illinois.edu/~yusra/download/30486.png It's possible that the coadd that gets built in pipe_tasks nopytest_test_coadd.py would work to test this as well (there are no unittests of GetCoaddAsTemplateTask). It's worth checking the layout of the patches/tracts that get built there. The existing `testCoaddPsf` in that test suite might be worth looking at as a comparison. Given the call signature for getTemplate's `run`, we can probably mock up the input coordinates to trigger the problem, assuming the test geometry is right. Also, this is independent of the problem that GetCoaddAsTemplateTask only operates on one tract. Yusra is going to file a separate ticket to deal with that problem. That would require reworking `run` to accept multiple tracts, and would need e.g. tract 9812 to be built for the above detector+visit example that crosses tract boundaries.
            Hide
            yusra Yusra AlSayyad added a comment -

             pipetask run -b /repo/main   -i HSC/runs/RC2/w_2021_14/DM-29528 -o  u/yusra/w_2021_14_templates  -p $OBS_SUBARU_DIR/pipelines/DRP.yaml#selectGoodSeeingVisits,templateGen -d "instrument='HSC' AND skymap='hsc_rings_v1' AND tract=9813 AND band='i' AND patch IN (60, 61, 62, 51, 52, 53, 42, 43, 44)"  -j  4
            

            is running now and should be done in 10 minutes or so. I picked those patches since I know they'll overlap that test detector. I'll kick off the rest as soon as it's finished.

            Note that I renamed my repo to u/yusra/w_2021_14_templates.

            Show
            yusra Yusra AlSayyad added a comment - pipetask run -b /repo/main -i HSC/runs/RC2/w_2021_14/DM-29528 -o u/yusra/w_2021_14_templates -p $OBS_SUBARU_DIR/pipelines/DRP.yaml#selectGoodSeeingVisits,templateGen -d "instrument='HSC' AND skymap='hsc_rings_v1' AND tract=9813 AND band='i' AND patch IN (60, 61, 62, 51, 52, 53, 42, 43, 44)" -j 4 is running now and should be done in 10 minutes or so. I picked those patches since I know they'll overlap that test detector. I'll kick off the rest as soon as it's finished. Note that I renamed my repo to u/yusra/w_2021_14_templates .
            sullivan Ian Sullivan made changes -
            Story Points 6
            sullivan Ian Sullivan made changes -
            Sprint AP S21-5 (April) [ 1084 ] AP S21-6 (May) [ 1088 ]
            sullivan Ian Sullivan made changes -
            Rank Ranked higher
            yusra Yusra AlSayyad made changes -
            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
            yusra Yusra AlSayyad made changes -
            Assignee John Parejko [ parejkoj ] Yusra AlSayyad [ yusra ]
            yusra Yusra AlSayyad made changes -
            Status To Do [ 10001 ] In Progress [ 3 ]
            sullivan Ian Sullivan made changes -
            Link This issue relates to DM-30496 [ DM-30496 ]
            sullivan Ian Sullivan made changes -
            Sprint AP S21-6 (May) [ 1088 ] AP S21-6 (May), AP F21-1 (June) [ 1088, 1096 ]
            yusra Yusra AlSayyad made changes -
            Epic Link PREOPS-450 [ 463950 ] PREOPS-477 [ 476909 ]
            sullivan Ian Sullivan made changes -
            Sprint AP S21-6 (May), AP F21-1 (June) [ 1088, 1096 ] AP S21-6 (May), AP F21-1 (June), AP F21-2 (July) [ 1088, 1096, 1102 ]
            yusra Yusra AlSayyad made changes -
            Attachment sourceDensity29310.png [ 51131 ]
            yusra Yusra AlSayyad made changes -
            Attachment sourceDensityDM-29310.png [ 51132 ]
            yusra Yusra AlSayyad made changes -
            Attachment sourceDensity29310.png [ 51131 ]
            yusra Yusra AlSayyad made changes -
            Attachment sourceDensityw_2021_28.png [ 51133 ]
            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.
            yusra Yusra AlSayyad made changes -
            Reviewers John Parejko [ parejkoj ]
            Status In Progress [ 3 ] In Review [ 10004 ]
            yusra Yusra AlSayyad made changes -
            Reviewers John Parejko [ parejkoj ] Ian Sullivan [ sullivan ]
            sullivan Ian Sullivan made changes -
            Sprint AP S21-6 (May), AP F21-1 (June), AP F21-2 (July) [ 1088, 1096, 1102 ] AP S21-6 (May), AP F21-1 (June), AP F21-2 (July), AP F21-3 (August) [ 1088, 1096, 1102, 1109 ]
            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.
            sullivan Ian Sullivan made changes -
            Status In Review [ 10004 ] Reviewed [ 10101 ]
            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.
            yusra Yusra AlSayyad made changes -
            Resolution Done [ 10000 ]
            Status Reviewed [ 10101 ] Done [ 10002 ]
            yusra Yusra AlSayyad made changes -
            Story Points 6 12
            yusra Yusra AlSayyad made changes -
            Team Alert Production [ 10300 ] Data Release Production [ 10301 ]

              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.