XMLWordPrintable

#### Details

• Type: Story
• Status: Done
• Resolution: Done
• Fix Version/s: None
• Component/s: None
• Labels:
None
• Story Points:
8
• Sprint:
DRP S19-2
• Team:
Data Release Production

#### 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
2. master_output.log
167 kB

#### Activity

Hide

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.

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

Show
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

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
Hide
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
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
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
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

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

Show
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

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
Hide

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

Show

#### People

Assignee:
Reporter:
Reviewers:
Ian Sullivan
Watchers: