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

Convert AssembleCoaddTasks to PipelineTasks with Shims

    Details

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

      Description

      Convert AssembleCoaddTasks to PipelineTask, using as much of existing code as possible with the shims that make gen3 quack like a gen2. Inputs are declared, so this is fine. They just don't fit into memory at once.

      Will require DM-13504, so I'm putting that change on this ticket.

      In the future, we'll refactor to split out the background-matching/mask-gen step from the stacking step. Now now.

        Attachments

        1. DM17045_output.log
          163 kB
          Yusra AlSayyad
        2. master_output.log
          167 kB
          Yusra AlSayyad

          Issue Links

            Activity

            Hide
            yusra Yusra AlSayyad added a comment -

            Hi Ian, You're welcome to get started reviewing this.  The unit test for DCR is failing, but I THINK I just did something stupid with the _init_()'s with the multiple inheritance. I don't have time to look into that until later today. 

            Branches on pipe_base, pipe_tasks.

            I've been informed that pipe_supertask is obsolete, so don't look in there

             

            Show
            yusra Yusra AlSayyad added a comment - Hi Ian, You're welcome to get started reviewing this.  The unit test for DCR is failing, but I THINK I just did something stupid with the _ init _() 's with the multiple inheritance. I don't have time to look into that until later today.  Branches on pipe_base, pipe_tasks. I've been informed that pipe_supertask is obsolete, so don't look in there  
            Hide
            yusra Yusra AlSayyad added a comment - - edited

            OK, So the problem is in test_dcrAssembleCoadd. The config is being passed as a positional arg rather than a keyword arg:

                    self.config = DcrAssembleCoaddConfig()
                    self.task = DcrAssembleCoaddTask(self.config) 

            This Gen3 PipelineTask ecosystem assumes Task's have no positional arguments. Your unit test isn't standard usage, but you're surely not the only one who has passed a config in like this. I bet there are notebooks out there that do it too, so I consider this (PipelineTask conversion) an API change. I'm deciding on how to best handle this. Catch any args not in the kwargs, put them in the kwargs, and raise a Deprecation warning?

            I'm really grateful for your unittest, because someone someday would be surprised about why their config wasn't being respected.

            Show
            yusra Yusra AlSayyad added a comment - - edited OK, So the problem is in test_dcrAssembleCoadd. The config is being passed as a positional arg rather than a keyword arg: self .config = DcrAssembleCoaddConfig() self .task = DcrAssembleCoaddTask( self .config) This Gen3 PipelineTask ecosystem assumes Task's have no positional arguments. Your unit test isn't standard usage, but you're surely not the only one who has passed a config in like this. I bet there are notebooks out there that do it too, so I consider this (PipelineTask conversion) an API change. I'm deciding on how to best handle this. Catch any args not in the kwargs, put them in the kwargs, and raise a Deprecation warning? I'm really grateful for your unittest, because someone someday would be surprised about why their config wasn't being respected.
            Hide
            sullivan Ian Sullivan added a comment - - edited

            I'm glad you figured it out, because I was having no luck with it, and was looking at all the wrong things. Yes, my two dcr-related unit tests do pass if I change that line to self.task = DcrAssembleCoaddTask(config=self.config). That might be the quick way forward for now, along with filing a new ticket (and RFC?) to deal with the more significant API change.

            Show
            sullivan Ian Sullivan added a comment - - edited I'm glad you figured it out, because I was having no luck with it, and was looking at all the wrong things. Yes, my two dcr-related unit tests do pass if I change that line to self.task = DcrAssembleCoaddTask(config=self.config) . That might be the quick way forward for now, along with filing a new ticket (and RFC?) to deal with the more significant API change.
            Hide
            sullivan Ian Sullivan added a comment -

            Looks good. I have tested dcrAssembleCoaddTask with these changes, and it appears to work as expected. I had mostly only requests for additional docstrings and code comments, which are hopefully easy for you to add.

            I have the one-line change to test_dcrAssembleCoadd.py on my machine that does the easy fix to make the tests pass. I can commit and push that if you like. I suggest you put off the harder changes of dealing with positional arguments like I had in that unit test for later work, but please do make the ticket for that now.

            Show
            sullivan Ian Sullivan added a comment - Looks good. I have tested dcrAssembleCoaddTask with these changes, and it appears to work as expected. I had mostly only requests for additional docstrings and code comments, which are hopefully easy for you to add. I have the one-line change to test_dcrAssembleCoadd.py on my machine that does the easy fix to make the tests pass. I can commit and push that if you like. I suggest you put off the harder changes of dealing with positional arguments like I had in that unit test for later work, but please do make the ticket for that now.
            Hide
            yusra Yusra AlSayyad added a comment -

            Thank you for the review!! No need to push the one-liner in the unit test (I have it locally).

            Show
            yusra Yusra AlSayyad added a comment - Thank you for the review!! No need to push the one-liner in the unit test (I have it locally).
            Hide
            yusra Yusra AlSayyad added a comment -

            About to merge. Note: This ticket changes the logging output for tests/nopytest_test_coadds.py. master_output.log DM17045_output.log I'm attaching the before and after for posterity. Snippet from the after:

            compareWarpAssembleCoadd INFO: Found 7 deepCoadd_directWarp
            compareWarpAssembleCoadd.assembleStaticSkyModel INFO: Weight of deepCoadd_psfMatchedWarp DataId(initialdata={'visit': 2, 'filter': 'r', 'patch': '0,0', 'tract': 0}, tag=set()) = 321.155
            compareWarpAssembleCoadd.assembleStaticSkyModel INFO: Weight of deepCoadd_psfMatchedWarp DataId(initialdata={'visit': 3, 'filter': 'r', 'patch': '0,0', 'tract': 0}, tag=set()) = 122.835
            compareWarpAssembleCoadd.assembleStaticSkyModel INFO: Weight of deepCoadd_psfMatchedWarp DataId(initialdata={'visit': 4, 'filter': 'r', 'patch': '0,0', 'tract': 0}, tag=set()) = 31.457
            compareWarpAssembleCoadd.assembleStaticSkyModel INFO: Weight of deepCoadd_psfMatchedWarp DataId(initialdata={'visit': 6, 'filter': 'r', 'patch': '0,0', 'tract': 0}, tag=set()) = 50.734
            compareWarpAssembleCoadd.assembleStaticSkyModel INFO: Weight of deepCoadd_psfMatchedWarp DataId(initialdata={'visit': 7, 'filter': 'r', 'patch': '0,0', 'tract': 0}, tag=set()) = 38.547
            compareWarpAssembleCoadd.assembleStaticSkyModel INFO: Weight of deepCoadd_psfMatchedWarp DataId(initialdata={'visit': 10, 'filter': 'r', 'patch': '0,0', 'tract': 0}, tag=set()) = 384.201
            compareWarpAssembleCoadd.assembleStaticSkyModel INFO: Weight of deepCoadd_psfMatchedWarp DataId(initialdata={'visit': 12, 'filter': 'r', 'patch': '0,0', 'tract': 0}, tag=set()) = 10.307
            

            Note it's not logging that it can't find the visits that don't cover the patch (e.g. 1, 5 ..)  anymore. This is because CompareWarp now passes the list of WarpRefs to its subtask's runDataRef rather than the original user-supplied list of CalexpRefs. They don't need to be grouped into visits and checked again, and it only tries to fetch the visits in the WarpRefList (and we've already confirmed the direct warp overlaps). 

            Show
            yusra Yusra AlSayyad added a comment - About to merge. Note: This ticket changes the logging output for tests/nopytest_test_coadds.py . master_output.log DM17045_output.log I'm attaching the before and after for posterity. Snippet from the after: compareWarpAssembleCoadd INFO: Found 7 deepCoadd_directWarp compareWarpAssembleCoadd.assembleStaticSkyModel INFO: Weight of deepCoadd_psfMatchedWarp DataId(initialdata={'visit': 2, 'filter': 'r', 'patch': '0,0', 'tract': 0}, tag=set()) = 321.155 compareWarpAssembleCoadd.assembleStaticSkyModel INFO: Weight of deepCoadd_psfMatchedWarp DataId(initialdata={'visit': 3, 'filter': 'r', 'patch': '0,0', 'tract': 0}, tag=set()) = 122.835 compareWarpAssembleCoadd.assembleStaticSkyModel INFO: Weight of deepCoadd_psfMatchedWarp DataId(initialdata={'visit': 4, 'filter': 'r', 'patch': '0,0', 'tract': 0}, tag=set()) = 31.457 compareWarpAssembleCoadd.assembleStaticSkyModel INFO: Weight of deepCoadd_psfMatchedWarp DataId(initialdata={'visit': 6, 'filter': 'r', 'patch': '0,0', 'tract': 0}, tag=set()) = 50.734 compareWarpAssembleCoadd.assembleStaticSkyModel INFO: Weight of deepCoadd_psfMatchedWarp DataId(initialdata={'visit': 7, 'filter': 'r', 'patch': '0,0', 'tract': 0}, tag=set()) = 38.547 compareWarpAssembleCoadd.assembleStaticSkyModel INFO: Weight of deepCoadd_psfMatchedWarp DataId(initialdata={'visit': 10, 'filter': 'r', 'patch': '0,0', 'tract': 0}, tag=set()) = 384.201 compareWarpAssembleCoadd.assembleStaticSkyModel INFO: Weight of deepCoadd_psfMatchedWarp DataId(initialdata={'visit': 12, 'filter': 'r', 'patch': '0,0', 'tract': 0}, tag=set()) = 10.307 Note it's not logging that it can't find the visits that don't cover the patch (e.g. 1, 5 ..)  anymore. This is because CompareWarp now passes the list of WarpRefs to its subtask's runDataRef rather than the original user-supplied list of CalexpRefs. They don't need to be grouped into visits and checked again, and it only tries to fetch the visits in the WarpRefList (and we've already confirmed the direct warp overlaps). 
            Hide
            yusra Yusra AlSayyad added a comment -

            See https://github.com/lsst-dm/demo_hsc_pipelinetask for configs to run this on ci_hsc

            pipetask  -b /project/yusra/ci_hsc/DATA/butler.yaml -i test/ci_hsc -d "Tract.tract=0 and Patch.patch=69" -o test/<outputCollection>  run -t assembleCoadd.CompareWarpAssembleCoaddTask --configfile CompareWarpAssembleCoaddTask:compareWarpAssembleCoadd.py

            Show
            yusra Yusra AlSayyad added a comment - See https://github.com/lsst-dm/demo_hsc_pipelinetask  for configs to run this on ci_hsc pipetask  -b /project/yusra/ci_hsc/DATA/butler.yaml -i test/ci_hsc -d "Tract.tract=0 and Patch.patch=69" -o test/<outputCollection>  run -t assembleCoadd.CompareWarpAssembleCoaddTask --configfile CompareWarpAssembleCoaddTask:compareWarpAssembleCoadd.py

              People

              • Assignee:
                yusra Yusra AlSayyad
                Reporter:
                yusra Yusra AlSayyad
                Reviewers:
                Ian Sullivan
                Watchers:
                Ian Sullivan, Yusra AlSayyad
              • Votes:
                0 Vote for this issue
                Watchers:
                2 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel