XMLWordPrintable

#### Details

• Type: Story
• Status: Done
• Resolution: Done
• Fix Version/s: None
• Component/s: None
• Labels:
• 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

1. 30500_54_templateGeom_annotated.pdf
46 kB
2. sourceDensityDM-29310.png
259 kB
3. sourceDensityw_2021_28.png
233 kB

#### Activity

No builds found.
Field Original Value New Value
Epic Link PREOPS-450 [ 463950 ]
 Assignee John Parejko [ parejkoj ]
 Sprint AP S21-5 (April) [ 1084 ] Team Alert Production [ 10300 ]
Hide
John Parejko added a comment -

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

Show
John Parejko added a comment - Following up on this: Bob Armstrong : do you have any information about the tract/patch that had failed?
Hide
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
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.
 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
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
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.
Hide
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 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

  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 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 .
 Story Points 6
 Sprint AP S21-5 (April) [ 1084 ] AP S21-6 (May) [ 1088 ]
 Rank Ranked higher
 Attachment 30500_54_templateGeom_annotated.pdf [ 49550 ]
Hide

Update: Data ID (visit=30500, ccd=54) triggers the “Could not find a valid average position for CoaddPsf”
30500_54_templateGeom_annotated.pdf

Show
 Assignee John Parejko [ parejkoj ] Yusra AlSayyad [ yusra ]
 Status To Do [ 10001 ] In Progress [ 3 ]
 Link This issue relates to DM-30496 [ DM-30496 ]
 Sprint AP S21-6 (May) [ 1088 ] AP S21-6 (May), AP F21-1 (June) [ 1088, 1096 ]
 Epic Link PREOPS-450 [ 463950 ] PREOPS-477 [ 476909 ]
 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 ]
 Attachment sourceDensity29310.png [ 51131 ]
 Attachment sourceDensityDM-29310.png [ 51132 ]
 Attachment sourceDensity29310.png [ 51131 ]
 Attachment sourceDensityw_2021_28.png [ 51133 ]
Hide

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 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.
 Reviewers John Parejko [ parejkoj ] Status In Progress [ 3 ] In Review [ 10004 ]
 Reviewers John Parejko [ parejkoj ] Ian Sullivan [ sullivan ]
 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
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
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.
 Status In Review [ 10004 ] Reviewed [ 10101 ]
Hide

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

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
Hide

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

Show
Yusra AlSayyad added a comment - Oh and this passed Jenkins and I think is ready to be merged.
 Resolution Done [ 10000 ] Status Reviewed [ 10101 ] Done [ 10002 ]
 Story Points 6 12
 Team Alert Production [ 10300 ] Data Release Production [ 10301 ]

#### People

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

#### Dates

Created:
Updated:
Resolved:

#### CI Builds

No builds found.