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

refactor coaddition code

    Details

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

      Description

      The HSC fork has coaddition code in two places: pipe_tasks and hscPipe. The code in hscPipe is what we use (though that depends on the code in pipe_tasks in places), while the code in pipe_tasks is more similar to what's currently on the LSST side.

      We want to bring the refactored version in hscPipe back to LSST, but we want to put it directly in pipe_tasks to remove the code duplication that currently exists on the HSC side.

      Work on this issue should begin with an RFC that details the proposed changes.

      Note that this should not bring over the "safe coadd clipping" code, which is DM-2915.

        Attachments

          Issue Links

            Activity

            Hide
            swinbank John Swinbank added a comment -

            Having spent a while getting to grips with this, I want to summarize where things stand and get a better sense of the intended scope of this issue.

            The coaddition code in hscPipe is built around stack.py. This provides StackTask as an an "all-in-one" solution for:

            • Selecting images to process (BaseSelectImagesTask, WcsSelectImagesTask);
            • Warping them to create coaddTempExp s (MakeCoaddTempExpTask);
            • Build a background reference for background-matched coaddition (BackgroundReferenceTask);
            • Assembling coadds, including optional background matching (SimpleAssembleCoaddTask);
            • Performing detection and measurement on coadds (ProcessCoaddTask).

            The following significant differences exist in these tasks as opposed to those on LSST:

            • There is no equivalent of BackgroundReferenceTask on LSST;
            • SimpleAssembleCoaddTask is a significant restructuring of LSST's AssembleCoaddTask, primarily to support a new way of performing background matching;
            • ProcessCoaddTask has been completely rewritten for hscPipe to use DetectCoaddSourcesTask. It no longer depends on ProcessImageTask;
            • There is no equivalent of the stack.py "all-in-one" task on LSST.

            In addition, there are many more minor structural differences, bug fixes and new features which have been introduced to code throughout the hierarchy touched by the above.

            To date, I have:

            • Agreed with Paul Price and Jim Bosch that it's not currently appropriate to port the HSC background matching code to LSST. Rather, we will preserve this for posterity on the HSC side, and use it a basis for future thinking about how to approach this on LSST.
            • Performed a comparison of code touched by the coaddition process to the end of (Simple)AssembleCoaddTask on HSC and LSST, accounted for all differences between them, and either cherry-picked those differences to an appropriate branch for this ticket (where minor), filed issues to bring over other, more significant, changes (eg DM-3243, DM-3258), or ensured that these changes are already covered by scheduled work elsewhere.
            • Performed some restructuring of LSST's AssembleCoaddTask so that it better corresponds to the SimpleAssembleCoaddTask on HSC, but without changing the background matching code (see above) or the "safe coadd clipping" (DM-2915).

            None of these changes involve significant changes to interface or functionality which seem worth of RFCing.

            My naive reading of this issue and its reference to "coaddition code" was that it referred to code which performed coaddition, and not to the subsequent processing of those coadds. However, given the mention of an RFC, and given that the changes here so far seem relatively modest, I'm wondering if the intention was also to bring across the redesigned ProcessCoaddTask.

            It looks as though bringing across the complete StackTask would be blocked by DM-2983 (it derives from BatchPoolTask for example). I've not investigated how easy it would be to create a purely serial version, but it doesn't seem worthwhile putting the effort into now just to re-port it to the HSC parallelization framework once that makes the transition.

            Jim Bosch, since you wrote the original description of this story, could you please clarify exactly what the success criteria here are, either with reference to my summary above or (if necessary!) by telling me what I've missed? Thanks!

            Show
            swinbank John Swinbank added a comment - Having spent a while getting to grips with this, I want to summarize where things stand and get a better sense of the intended scope of this issue. The coaddition code in hscPipe is built around stack.py . This provides StackTask as an an "all-in-one" solution for: Selecting images to process ( BaseSelectImagesTask , WcsSelectImagesTask ); Warping them to create coaddTempExp s ( MakeCoaddTempExpTask ); Build a background reference for background-matched coaddition ( BackgroundReferenceTask ); Assembling coadds, including optional background matching ( SimpleAssembleCoaddTask ); Performing detection and measurement on coadds ( ProcessCoaddTask ). The following significant differences exist in these tasks as opposed to those on LSST: There is no equivalent of BackgroundReferenceTask on LSST; SimpleAssembleCoaddTask is a significant restructuring of LSST's AssembleCoaddTask , primarily to support a new way of performing background matching; ProcessCoaddTask has been completely rewritten for hscPipe to use DetectCoaddSourcesTask . It no longer depends on ProcessImageTask ; There is no equivalent of the stack.py "all-in-one" task on LSST. In addition, there are many more minor structural differences, bug fixes and new features which have been introduced to code throughout the hierarchy touched by the above. To date, I have: Agreed with Paul Price and Jim Bosch that it's not currently appropriate to port the HSC background matching code to LSST. Rather, we will preserve this for posterity on the HSC side, and use it a basis for future thinking about how to approach this on LSST. Performed a comparison of code touched by the coaddition process to the end of (Simple)AssembleCoaddTask on HSC and LSST, accounted for all differences between them, and either cherry-picked those differences to an appropriate branch for this ticket (where minor), filed issues to bring over other, more significant, changes (eg DM-3243 , DM-3258 ), or ensured that these changes are already covered by scheduled work elsewhere. Performed some restructuring of LSST's AssembleCoaddTask so that it better corresponds to the SimpleAssembleCoaddTask on HSC, but without changing the background matching code (see above) or the "safe coadd clipping" ( DM-2915 ). None of these changes involve significant changes to interface or functionality which seem worth of RFCing. My naive reading of this issue and its reference to "coaddition code" was that it referred to code which performed coaddition, and not to the subsequent processing of those coadds. However, given the mention of an RFC, and given that the changes here so far seem relatively modest, I'm wondering if the intention was also to bring across the redesigned ProcessCoaddTask . It looks as though bringing across the complete StackTask would be blocked by DM-2983 (it derives from BatchPoolTask for example). I've not investigated how easy it would be to create a purely serial version, but it doesn't seem worthwhile putting the effort into now just to re-port it to the HSC parallelization framework once that makes the transition. Jim Bosch , since you wrote the original description of this story, could you please clarify exactly what the success criteria here are, either with reference to my summary above or (if necessary!) by telling me what I've missed? Thanks!
            Hide
            jbosch Jim Bosch added a comment -

            It sounds like I mostly just overestimated the scale of the changes (and hence the need for an RFC). I also did not intend for any porting of stack.py to be in scope (because of the blocker you mentioned); I think it's best to limit this to changes to MakeCoaddTempExpTask and [Simple]AssembleCoaddTask.

            Show
            jbosch Jim Bosch added a comment - It sounds like I mostly just overestimated the scale of the changes (and hence the need for an RFC). I also did not intend for any porting of stack.py to be in scope (because of the blocker you mentioned); I think it's best to limit this to changes to MakeCoaddTempExpTask and [Simple] AssembleCoaddTask .
            Hide
            swinbank John Swinbank added a comment -

            Thanks Jim! I'll tidy up what I have then and put it out for review.

            I guess we will want another story to cover the porting of ProcessCoaddTask? I don't think that's covered elsewhere, and I'm not immediately sure whether we want to adopt the HSC version wholesale for LSST or if there are more subtleties to consider.

            Show
            swinbank John Swinbank added a comment - Thanks Jim! I'll tidy up what I have then and put it out for review. I guess we will want another story to cover the porting of ProcessCoaddTask ? I don't think that's covered elsewhere, and I'm not immediately sure whether we want to adopt the HSC version wholesale for LSST or if there are more subtleties to consider.
            Hide
            jbosch Jim Bosch added a comment -

            I think we actually want to drop ProcessCoaddTask. I propose:

            • When we port stack.py, we have it do detection and background-subtraction (as it does on the HSC side), but not deblending, measurement, or anything else. That will make stack.py compatible with the multi-band coadd processing, while leaving the old LSST-side option to run MakeCoaddTempExp and AssembleCoadd manually to feed ProcessCoaddTask.
            • We remove the existing ProcessCoaddTask as part of the audit and refactor of DLP-495.
            Show
            jbosch Jim Bosch added a comment - I think we actually want to drop ProcessCoaddTask . I propose: When we port stack.py , we have it do detection and background-subtraction (as it does on the HSC side), but not deblending, measurement, or anything else. That will make stack.py compatible with the multi-band coadd processing, while leaving the old LSST-side option to run MakeCoaddTempExp and AssembleCoadd manually to feed ProcessCoaddTask . We remove the existing ProcessCoaddTask as part of the audit and refactor of DLP-495.
            Hide
            swinbank John Swinbank added a comment - - edited

            Paul Price, would you mind taking a look at this please?

            It's worth scanning the discussion above to see the scope of this issue. To summarize, the aim is to make LSST's coaddition code look as similar as possible to HSC's equivalent through to the end of AssembleCoaddTask, but without porting the "safe coadd clipping" (which is covered by DM-2915) or reworked background matching. This includes changes to LSST's AssembleCoaddTask, CoaddBaseTask, CoaddInputRecorder and MakeCoaddTempExpTask and/or their associated config classes. It covers work performed on HSC-804 and parts of HSC-138 (see also DM-245 and DM-2674), HSC-669 (see also DM-3136), HSC-897 (see also DM-833), HSC-1112 (the remainder of which does not apply to LSST) and HSC-1126 (see also DM-3264), as well as several unticketed commits on HSC and a some home-grown restructuring.

            Everything is on tickets/DM-2980 in pipe_tasks.

            Show
            swinbank John Swinbank added a comment - - edited Paul Price , would you mind taking a look at this please? It's worth scanning the discussion above to see the scope of this issue. To summarize, the aim is to make LSST's coaddition code look as similar as possible to HSC's equivalent through to the end of AssembleCoaddTask , but without porting the "safe coadd clipping" (which is covered by DM-2915 ) or reworked background matching. This includes changes to LSST's AssembleCoaddTask , CoaddBaseTask , CoaddInputRecorder and MakeCoaddTempExpTask and/or their associated config classes. It covers work performed on HSC-804 and parts of HSC-138 (see also DM-245 and DM-2674 ), HSC-669 (see also DM-3136 ), HSC-897 (see also DM-833 ), HSC-1112 (the remainder of which does not apply to LSST) and HSC-1126 (see also DM-3264 ), as well as several unticketed commits on HSC and a some home-grown restructuring. Everything is on tickets/DM-2980 in pipe_tasks .
            Hide
            price Paul Price added a comment -

            Looks good. Trivial comments made on github pull request: https://github.com/lsst/pipe_tasks/pull/12

            Show
            price Paul Price added a comment - Looks good. Trivial comments made on github pull request: https://github.com/lsst/pipe_tasks/pull/12
            Hide
            swinbank John Swinbank added a comment -

            Thanks Paul!

            Show
            swinbank John Swinbank added a comment - Thanks Paul!

              People

              • Assignee:
                swinbank John Swinbank
                Reporter:
                jbosch Jim Bosch
                Reviewers:
                Paul Price
                Watchers:
                Jim Bosch, John Swinbank, Paul Price
              • Votes:
                0 Vote for this issue
                Watchers:
                3 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel