# Convert DCR code to use Tasks

XMLWordPrintable

## Details

• Type: Story
• Status: Done
• Resolution: Done
• Fix Version/s: None
• Component/s:
• Labels:
• Story Points:
8
• Sprint:
Alert Production S17 - 3, Alert Production S17 - 4, Alert Production S17 - 5, Alert Production S17 - 6, Alert Production F17 - 7, Alert Production F17 - 8, Alert Production F17 - 9, Alert Production F17 - 10, AP S18-1, AP S18-2, AP S18-3, AP S18-4, AP S18-5, AP S18-6, AP F18-1, AP F18-2
• Team:

## Description

The DCR code should be written so that the classes inherit from Tasks, and make use of those features.

## Activity

Hide
Ian Sullivan added a comment -

Looks like this is needed for DM-10522, so I'm moving it up the queue.

Show
Ian Sullivan added a comment - Looks like this is needed for DM-10522 , so I'm moving it up the queue.
Hide
Ian Sullivan added a comment -

This is the complete initial implementation of making DCR coadds. Because the code is in pipe_tasks I have followed the documentation style of assembleCoadd, but I would be perfectly happy to switch to numpydoc format instead.

Show
Ian Sullivan added a comment - This is the complete initial implementation of making DCR coadds. Because the code is in pipe_tasks I have followed the documentation style of assembleCoadd, but I would be perfectly happy to switch to numpydoc format instead.
Hide
Paul Price added a comment -

I've done a first pass review on GitHub with many comments, ranging from important design questions to trivial typos. You shouldn't read anything into the large number of comments except that you asked me to review a lot of code: it's a real good start. Please let me know if you need further explanation on any of the comments.

I think the design of this code suffers from the lack of a class that encapsulates the DCR coadd. You should build DcrAssembleCoaddTask around that class: assembling an instance of it, then writing it; this would greatly simplify things by letting the Task do the heavy lifting, while letting the DcrCoadd handle the accounting; it would also help address the problems in the data model. Here's the start of an example API:

 class DcrCoadd(object):  def __init__(self, bbox, wcs, nSubbands): # Ctor  @classmethod  def fromImage(self, exposure, nSubbands): # Ctor; from dcrDivideCoadd  def __getitem__(self, ii): # Return band ii  def __len__(self): # Return nSubbands  def getTemplate(self): # Return sum of all bands  @classmethod  def readFits(self, filename): # Read from FITS  def writeFits(self, filename): # Write to FITS  def calculateResiduals(self, exposure): # From dcrResiduals 

Show
Paul Price added a comment - I've done a first pass review on GitHub with many comments, ranging from important design questions to trivial typos. You shouldn't read anything into the large number of comments except that you asked me to review a lot of code: it's a real good start. Please let me know if you need further explanation on any of the comments. I think the design of this code suffers from the lack of a class that encapsulates the DCR coadd. You should build DcrAssembleCoaddTask around that class: assembling an instance of it, then writing it; this would greatly simplify things by letting the Task do the heavy lifting, while letting the DcrCoadd handle the accounting; it would also help address the problems in the data model. Here's the start of an example API: class DcrCoadd(object): def __init__(self, bbox, wcs, nSubbands): # Ctor @classmethod def fromImage(self, exposure, nSubbands): # Ctor; from dcrDivideCoadd def __getitem__(self, ii): # Return band ii def __len__(self): # Return nSubbands def getTemplate(self): # Return sum of all bands @classmethod def readFits(self, filename): # Read from FITS def writeFits(self, filename): # Write to FITS def calculateResiduals(self, exposure): # From dcrResiduals
Hide
Paul Price added a comment -

In my second pass review, I noticed that you still don't have a class encapsulating the DCR coadd. I still think that would help clean up a lot of the code, and I am reluctant to recommend merging before that is remedied.
You should also read the Dev Guide on numpydoc, as you're nonconformal throughout.

Show
Paul Price added a comment - In my second pass review, I noticed that you still don't have a class encapsulating the DCR coadd. I still think that would help clean up a lot of the code, and I am reluctant to recommend merging before that is remedied. You should also read the Dev Guide on numpydoc , as you're nonconformal throughout.
Hide
Ian Sullivan added a comment -

I've refactored the code to make the DCR coadd it's own class, and have tried to separate the functionality of the new DcrModel from DcrAssembleCoaddTask. I believe the new class does make the code simpler and easier to follow, so thank you for encouraging me to write it that way. I debated the placement of several methods in the Task or in the class, and am happy to consider alternate organizations if you have any suggestions.

While I have tried to address most of your concerns, I have left writing new unit tests for future work on new tickets. I certainly agree about the difficulty to maintain code without proper tests, and writing those will be a priority, but John Swinbank has asked me to work on those on separate tickets for better accounting.

Show
Ian Sullivan added a comment - I've refactored the code to make the DCR coadd it's own class, and have tried to separate the functionality of the new DcrModel from DcrAssembleCoaddTask. I believe the new class does make the code simpler and easier to follow, so thank you for encouraging me to write it that way. I debated the placement of several methods in the Task or in the class, and am happy to consider alternate organizations if you have any suggestions. While I have tried to address most of your concerns, I have left writing new unit tests for future work on new tickets. I certainly agree about the difficulty to maintain code without proper tests, and writing those will be a priority, but John Swinbank has asked me to work on those on separate tickets for better accounting.
Hide
Paul Price added a comment -

Review completed on GitHub.

My last major concern is I/O: I think you should add readFits and writeFits methods to your DcrModel class that will write the data as a single file, so everything stays together. Then you can have the butler read/write this class directly.

Show
Paul Price added a comment - Review completed on GitHub. My last major concern is I/O: I think you should add readFits and writeFits methods to your DcrModel class that will write the data as a single file, so everything stays together. Then you can have the butler read/write this class directly.
Hide
Paul Price added a comment -

Ian Sullivan points out that saving the components of the DcrModel in separate FITS files makes it easy to do multiband measurements (by setting coaddName=dcr and using --id subfilter=0..2). I'm not perfectly happy about this, but it seems like a reasonable solution for the time being. In that case, the existing solution for I/O is reasonable, so I withdraw my objections.

Show
Paul Price added a comment - Ian Sullivan points out that saving the components of the DcrModel in separate FITS files makes it easy to do multiband measurements (by setting coaddName=dcr and using --id subfilter=0..2 ). I'm not perfectly happy about this, but it seems like a reasonable solution for the time being. In that case, the existing solution for I/O is reasonable, so I withdraw my objections.

## People

• Assignee:
Ian Sullivan
Reporter:
Ian Sullivan
Reviewers:
Paul Price
Watchers:
Ian Sullivan, John Swinbank, Paul Price, Simon Krughoff