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

Convert DCR code to use Tasks

    Details

    • 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:
      Alert Production

      Description

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

        Attachments

          Issue Links

            Activity

            Hide
            sullivan Ian Sullivan added a comment -

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

            Show
            sullivan Ian Sullivan added a comment - Looks like this is needed for DM-10522 , so I'm moving it up the queue.
            Hide
            sullivan 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
            sullivan 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
            price 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
            price 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
            price 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
            price 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
            sullivan 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
            sullivan 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
            price 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
            price 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
            price 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
            price 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:
                sullivan Ian Sullivan
                Reporter:
                sullivan Ian Sullivan
                Reviewers:
                Paul Price
                Watchers:
                Ian Sullivan, John Swinbank, Paul Price, Simon Krughoff
              • Votes:
                0 Vote for this issue
                Watchers:
                4 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel