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

Get ImageDifferenceTask running again

    Details

      Description

      ImageDifferenceTask doesn't run. The issues I've seen so far are related to the measurement overhaul. This ticket will capture the one-off updates needed to get this running again.

      Appropriate Bugs and Papercuts epic?

        Attachments

          Issue Links

            Activity

            Hide
            yusra Yusra AlSayyad added a comment - - edited

            Comments:

            To goal of this ticket was to get the untested imageDifferenceTask running again. This ticket was tested on decam HiTs data and Stripe 82. This ticket is an intermediate step before the following work:

            • Image Difference Task overhaul. DM-3704
              • Refactor
              • Add unit tests
              • Test and include support in additional obs packages
            • Dipole Measurement Task: DM-3515 (The dependency was switched, as I got this running with the old DipoleMeasurement with a few minimal changes.)
            • Fix Register Task. (Need new ticket). It consistently ruins the registration and is turned off by default in this ticket.

            Russell, do you have time to review? Changes are on branch tickets/DM-3213 (NOTE: ignore the user branches as they present a different solution)

            obs_decam yusra$ git diff master.. --stat
             policy/DecamMapper.paf | 205 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
             1 file changed, 205 insertions(+)
             
            obs_sdss yusra$ git diff master.. --stat
             policy/SdssMapper.paf | 112 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
             1 file changed, 112 insertions(+)
             
            pipe_tasks yusra$ git diff master.. --stat
             python/lsst/pipe/tasks/diffimGetTemplate.py | 174 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
             python/lsst/pipe/tasks/imageDifference.py   | 200 ++++++++++++++++++++++++++++---------------------------------------------------------------
             2 files changed, 236 insertions(+), 138 deletions(-)
             
            ip_diffim yusra$ git diff master.. --stat
             python/lsst/ip/diffim/diaCatalogSourceSelector.py | 4 ++--
             python/lsst/ip/diffim/diffimTools.py              | 5 ++++-
             python/lsst/ip/diffim/dipoleMeasurement.py        | 9 ++++++---
             python/lsst/ip/diffim/psfMatch.py                 | 2 +-
             4 files changed, 13 insertions(+), 7 deletions(-)
            

            Given that this is an intermediate step, some suggestions I will delay to DM-3704. The ticket page has a running to do list.

            Aside from bug fixes, there was one substantive change in this ticket: Moving getTemplate to its own subtask. I would appreciate your advice in particular on
            1) the task names:
            module: diffimGetTemplate.py

            • GetTemplateCoaddTask (Default coadd templates)
            • GetTemplateSameCcdCalexpTask
              • Shared between Winter2013ImageDifferenceTask and obs_decam processing of HiTs data
              • visit is giving in the config. gets same ccdnum as the the science image

            2) How the configs are passed betweenthe subtask and ImageDifferenceTask.

            • Right now they are manually set which seems clunky:

              	self.makeSubtask("getTemplate")
                      if self.config.getTemplate == "GetTemplateSameCcdCalexpTask":
                          self.getTemplate.config.doAddCalexpBackground = self.config.doAddCalexpBackground
              

            • In the refactor we also want the names of the image differences to depend on the types of templates used. Now we have deepDiff and goodSeeingDiff, but arguably the diffs with calexp templates should be called just diff, or singleFrameDiff. I'm thinking baout where this should be controlled from.
            Show
            yusra Yusra AlSayyad added a comment - - edited Comments: To goal of this ticket was to get the untested imageDifferenceTask running again. This ticket was tested on decam HiTs data and Stripe 82. This ticket is an intermediate step before the following work: Image Difference Task overhaul. DM-3704 Refactor Add unit tests Test and include support in additional obs packages Dipole Measurement Task: DM-3515 (The dependency was switched, as I got this running with the old DipoleMeasurement with a few minimal changes.) Fix Register Task. (Need new ticket). It consistently ruins the registration and is turned off by default in this ticket. Russell, do you have time to review? Changes are on branch tickets/ DM-3213 (NOTE: ignore the user branches as they present a different solution) obs_decam yusra$ git diff master.. --stat policy/DecamMapper.paf | 205 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 205 insertions(+)   obs_sdss yusra$ git diff master.. --stat policy/SdssMapper.paf | 112 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 112 insertions(+)   pipe_tasks yusra$ git diff master.. --stat python/lsst/pipe/tasks/diffimGetTemplate.py | 174 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ python/lsst/pipe/tasks/imageDifference.py | 200 ++++++++++++++++++++++++++++--------------------------------------------------------------- 2 files changed, 236 insertions(+), 138 deletions(-)   ip_diffim yusra$ git diff master.. --stat python/lsst/ip/diffim/diaCatalogSourceSelector.py | 4 ++-- python/lsst/ip/diffim/diffimTools.py | 5 ++++- python/lsst/ip/diffim/dipoleMeasurement.py | 9 ++++++--- python/lsst/ip/diffim/psfMatch.py | 2 +- 4 files changed, 13 insertions(+), 7 deletions(-) Given that this is an intermediate step, some suggestions I will delay to DM-3704 . The ticket page has a running to do list. Aside from bug fixes, there was one substantive change in this ticket: Moving getTemplate to its own subtask. I would appreciate your advice in particular on 1) the task names: module: diffimGetTemplate.py GetTemplateCoaddTask (Default coadd templates) GetTemplateSameCcdCalexpTask Shared between Winter2013ImageDifferenceTask and obs_decam processing of HiTs data visit is giving in the config. gets same ccdnum as the the science image 2) How the configs are passed betweenthe subtask and ImageDifferenceTask. Right now they are manually set which seems clunky: self.makeSubtask("getTemplate") if self.config.getTemplate == "GetTemplateSameCcdCalexpTask": self.getTemplate.config.doAddCalexpBackground = self.config.doAddCalexpBackground In the refactor we also want the names of the image differences to depend on the types of templates used. Now we have deepDiff and goodSeeingDiff, but arguably the diffs with calexp templates should be called just diff, or singleFrameDiff. I'm thinking baout where this should be controlled from.
            Hide
            yusra Yusra AlSayyad added a comment - - edited

            To test on decam HiTs data (wherein each visit is aligned – like the Winter2013 DC):

            setup config file like:

            $ cat diffimconfig.py
            from lsst.pipe.tasks.diffimGetTemplate import GetTemplateSameCcdCalexpTask
            config.getTemplate.retarget(GetTemplateSameCcdCalexpTask)
            config.getTemplate.visit = 288976 #or any other visit with calexps
            

            Run like:

            $PIPE_TASKS_DIR/bin/imageDifference.py /lsst8/image_diff/calexp_dir_Blind14A_10  --id visit=289820 ccdnum=10 --configfile diffimconfig.py --output /path/to/output --clobber-config
            

            As of writing calexps for ccdnum=1..30 available.

            To test on Stripe 82 data:

            $PIPE_TASKS_DIR/bin/imageDifference.py  /lsst8/yusra/RX2129_v11/calexp_dir/  --id camcol=4 field=105 filter=r run=4192  --config doSelectSources=False convolveTemplate=False --output /lsst8/yusra/RX2129_v11/coadd_r_dir  --clobber-config
            

            Show
            yusra Yusra AlSayyad added a comment - - edited To test on decam HiTs data (wherein each visit is aligned – like the Winter2013 DC): setup config file like: $ cat diffimconfig.py from lsst.pipe.tasks.diffimGetTemplate import GetTemplateSameCcdCalexpTask config.getTemplate.retarget(GetTemplateSameCcdCalexpTask) config.getTemplate.visit = 288976 #or any other visit with calexps Run like: $PIPE_TASKS_DIR/bin/imageDifference.py /lsst8/image_diff/calexp_dir_Blind14A_10 --id visit=289820 ccdnum=10 --configfile diffimconfig.py --output /path/to/output --clobber-config As of writing calexps for ccdnum=1..30 available. To test on Stripe 82 data : $PIPE_TASKS_DIR/bin/imageDifference.py /lsst8/yusra/RX2129_v11/calexp_dir/ --id camcol=4 field=105 filter=r run=4192 --config doSelectSources=False convolveTemplate=False --output /lsst8/yusra/RX2129_v11/coadd_r_dir --clobber-config
            Hide
            price Paul Price added a comment -

            I'm guessing this is just a temporary workaround, but I don't like having to specify a template visit in the configuration, because the configuration then has to change from pointing to pointing. We've done workarounds in a similar way for HSC in the past, and found it was not good. It's better to supply things like this via a command-line argument or some other mechanism.

            Show
            price Paul Price added a comment - I'm guessing this is just a temporary workaround, but I don't like having to specify a template visit in the configuration, because the configuration then has to change from pointing to pointing. We've done workarounds in a similar way for HSC in the past, and found it was not good. It's better to supply things like this via a command-line argument or some other mechanism.
            Hide
            yusra Yusra AlSayyad added a comment -

            Thank you Russell for the all suggestions on the getTemplate subtask and instructions on how to specify the template visit on the command line. One additional decision was what to do if more than one template visit number is specified: with this implementation GetCalexpAsTemplate.run() warns that it will use the first visit specified. Like we talked about, I changed the names and moved getTemplate.py to ip_diffim. Re: configs, I changed my mind on the attempt to tether doAddCalexpBackground in ImageDifference and getTemplate. It's conceivable that a user would intentionally want to change one without the other.

            Now you would test on HiTs data like:

            $PIPE_TASKS_DIR/bin/imageDifference.py /lsst8/image_diff/calexp_dir_Blind14A_10  --id visit=289820 ccdnum=10   --templateId visit=288976 --configfile diffimconfig.py --output /path/to/output --clobber-config
            

            All changes are in tickets/DM-3213 in pipe_tasks, ip_diffim, obs_sdss, and obs_decam. It's ready for review when you get a chance. It'd be great if it were available when the bootcamp attendees rebuild their master stacks next Monday.

            Show
            yusra Yusra AlSayyad added a comment - Thank you Russell for the all suggestions on the getTemplate subtask and instructions on how to specify the template visit on the command line. One additional decision was what to do if more than one template visit number is specified: with this implementation GetCalexpAsTemplate.run() warns that it will use the first visit specified. Like we talked about, I changed the names and moved getTemplate.py to ip_diffim. Re: configs, I changed my mind on the attempt to tether doAddCalexpBackground in ImageDifference and getTemplate. It's conceivable that a user would intentionally want to change one without the other. Now you would test on HiTs data like: $PIPE_TASKS_DIR/bin/imageDifference.py /lsst8/image_diff/calexp_dir_Blind14A_10 --id visit=289820 ccdnum=10 --templateId visit=288976 --configfile diffimconfig.py --output /path/to/output --clobber-config All changes are in tickets/ DM-3213 in pipe_tasks, ip_diffim, obs_sdss, and obs_decam. It's ready for review when you get a chance. It'd be great if it were available when the bootcamp attendees rebuild their master stacks next Monday.
            Hide
            rowen Russell Owen added a comment -

            This adds some very nice improvements and clarifications. I just have a few minor comments and a request for more documentation.

            pipe_tasks

            imageDifference.py

            • afwImage is imported but not used
            • GetCoaddAsTemplateTask, GetCalexpAsTemplateTask can be imported from lsst.im.diffim; you don't need to go down into getTemplate to get to them, and doing so makes the code a bit less clear
            • I suggest moving "import lsst.ip.diffim.utils as diUtils" to the top; the most important reason is to catch errors that prevent the module from being imported early, but it also reduces repetition and makes dependencies more obvious

            ip_diffIm

            getTemplate.py:

            GetTemplateCoaddTask and GetCalexpAsTemplateTask

            • These tasks are missing the doxygen comments that makes them show up on the main task documentation page. Please add that to make the tasks visible.
            • The main doc strings are too brief; please fill these out as per our standard for tasks (including a brief example of how to run the task). If you are out of time for that now, please file a ticket so it does not get forgotten.

            I suggest adding the configs to _all_ (in the past I have argued that it's just as good or better to get to the configs via the task's ConfigureClass method, but others disagree, and it certainly is true that it can be convenient to get to the configs directly).

            imagePsfMatch.py:

            ImagePsfMatchTask.makeCandidateList argument "candidateList": please document the two accepted data types in the @param description. When you refactor this code I hope you'll find you can require one of the two data types and make the user convert the other one, as it would simplify the API and the code. Also, with any luck you can clarify a function called makeCandidateList that take one kind of candidate list and return another – I find that really confusing, but not worth fixing on this ticket.

            psfMatch.py

            • pex.exceptions is imported but never used
            Show
            rowen Russell Owen added a comment - This adds some very nice improvements and clarifications. I just have a few minor comments and a request for more documentation. pipe_tasks imageDifference.py afwImage is imported but not used GetCoaddAsTemplateTask, GetCalexpAsTemplateTask can be imported from lsst.im.diffim; you don't need to go down into getTemplate to get to them, and doing so makes the code a bit less clear I suggest moving "import lsst.ip.diffim.utils as diUtils" to the top; the most important reason is to catch errors that prevent the module from being imported early, but it also reduces repetition and makes dependencies more obvious ip_diffIm getTemplate.py: GetTemplateCoaddTask and GetCalexpAsTemplateTask These tasks are missing the doxygen comments that makes them show up on the main task documentation page. Please add that to make the tasks visible. The main doc strings are too brief; please fill these out as per our standard for tasks (including a brief example of how to run the task). If you are out of time for that now, please file a ticket so it does not get forgotten. I suggest adding the configs to _ all _ (in the past I have argued that it's just as good or better to get to the configs via the task's ConfigureClass method, but others disagree, and it certainly is true that it can be convenient to get to the configs directly). imagePsfMatch.py: ImagePsfMatchTask.makeCandidateList argument "candidateList": please document the two accepted data types in the @param description. When you refactor this code I hope you'll find you can require one of the two data types and make the user convert the other one, as it would simplify the API and the code. Also, with any luck you can clarify a function called makeCandidateList that take one kind of candidate list and return another – I find that really confusing, but not worth fixing on this ticket. psfMatch.py pex.exceptions is imported but never used
            Hide
            yusra Yusra AlSayyad added a comment -

            Thanks for the review.
            Work to be continued...

            • The doxygen task documentation for imageDifference and its subtasks will be part of DM-3704, so that all the diffIm tasks will hit the main task page at once.
            • Cleaning up dipole centroids and classification in DipoleMeasurementTask is next: DM-3515
            Show
            yusra Yusra AlSayyad added a comment - Thanks for the review. Work to be continued... The doxygen task documentation for imageDifference and its subtasks will be part of DM-3704 , so that all the diffIm tasks will hit the main task page at once. Cleaning up dipole centroids and classification in DipoleMeasurementTask is next: DM-3515

              People

              • Assignee:
                yusra Yusra AlSayyad
                Reporter:
                yusra Yusra AlSayyad
                Reviewers:
                Russell Owen
                Watchers:
                David Reiss, Hsin-Fang Chiang, Ian Sullivan, Paul Price, Russell Owen, Simon Krughoff, Yusra AlSayyad
              • Votes:
                0 Vote for this issue
                Watchers:
                7 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel