DM-10805: Spatially-varying ZOGY option #66

djreiss · 2017-06-06T21:39:13Z

Include unit tests for new task

yalsayyad · 2017-07-05T19:03:37Z

python/lsst/ip/diffim/zogy.py

@@ -288,8 +319,8 @@ def computePrereqs(self, psf1=None, psf2=None, padSize=0):
        padSize = self.padSize if padSize is None else padSize
        Pr, Pn = psf1, psf2
        if padSize > 0:
-            Pr = ZogyTask._padPsfToSize(psf1, (padSize, padSize))
-            Pn = ZogyTask._padPsfToSize(psf2, (padSize, padSize))
+            Pr = ZogyTask._padPsfToSize(psf1, (psf1.shape[0]+padSize, psf1.shape[0]+padSize))


Did you mean: (psf1.shape[0] + padSize, psf1.shape[1] + padSize) or is there something special about the x dimension. Please comment if so.

This arose from a desire to force it to be square. However, that is enforced below, so I will update it here to avoid confusion.

yalsayyad · 2017-07-05T19:14:29Z

python/lsst/ip/diffim/zogy.py

+            results = task.run(scienceExposure, template=templateExposure, inImageSpace=inImageSpace,
+                               doScorr=doPreConvolve, forceEvenSized=True)
+            results.D = results.exposure
+            # The CoaddPsf apparently cannot be used for detection as it doesn't have a


Is this comment about CoaddPsf still true?

I don't think it was ever true. We have used CoaddPsf for detection from its genesis.
If I'm wrong, please file a ticket on meas_algorithms.

My understanding from looking at the code was that it computes a single psf from the averagePosition and uses that for detection. But it is still a good point that the comment is untrue. I've updated the comment wrt my understanding on this.

yalsayyad · 2017-07-09T00:44:55Z

python/lsst/ip/diffim/zogy.py

+    def __init__(self, *args, **kwargs):
+        ImagePsfMatchTask.__init__(self, *args, **kwargs)
+
+    def setDefaults(self):


Does this function do anything? I thought only Configs run setDefaults.

Also, redundant, so removed.

yalsayyad · 2017-07-09T00:51:42Z

python/lsst/ip/diffim/zogy.py

+
+        # Make sure masks of input images are propagated to diffim
+        mask = results.D.getMaskedImage().getMask()
+        badBits = mask.getPlaneBitMask(['UNMASKEDNAN', 'NO_DATA', 'BAD', 'EDGE', 'SUSPECT', 'CR', 'SAT'])


badBits not used.

yalsayyad · 2017-07-09T00:54:14Z

python/lsst/ip/diffim/zogy.py

+        # Make sure masks of input images are propagated to diffim
+        mask = results.D.getMaskedImage().getMask()
+        badBits = mask.getPlaneBitMask(['UNMASKEDNAN', 'NO_DATA', 'BAD', 'EDGE', 'SUSPECT', 'CR', 'SAT'])
+        badBitsNan = mask.getPlaneBitMask(['UNMASKEDNAN'])


See comment on jira ticket about UNMASKEDNAN. Not sure if calexps/coadds are guaranteed to have this. Check to see if this plane is present first before using it

yalsayyad · 2017-07-09T00:58:13Z

python/lsst/ip/diffim/zogy.py

+    def subtractExposures(self, templateExposure, scienceExposure,
+                          doWarping=True, spatiallyVarying=True, inImageSpace=False,
+                          doPreConvolve=False):
+


Needs docstring (esp. to make it clear that this function modifies the input exposures)

I will surely add the docstring, and you are correct, the means of the input images are subtracted inplace, so I will note that.

yalsayyad · 2017-07-09T01:23:36Z

python/lsst/ip/diffim/zogy.py

+                                                             destBBox=scienceExposure.getBBox())
+
+                templateExposure.setPsf(psfWarped)
+                templateExposure.writeFits('WARPEDTEMPLATE_ZOGY.fits')


Tasks shouldn't write exposures to the current working directory. If you really need this, use the lsstDebug framework to tuck it in an if statement so that it only writes out if requested.

Whoops, meant to remove that, thanks for catching it.

yalsayyad · 2017-07-09T01:26:10Z

python/lsst/ip/diffim/zogy.py

+        gm(results.D).getArray()[np.isnan(ga(scienceExposure))] |= badBitsNan
+        gm(results.D).getArray()[np.isnan(ga(templateExposure))] |= badBitsNan
+
+        #results = pipeBase.Struct(exposure=D)


Remove this line and 1074.

yalsayyad · 2017-07-10T15:22:05Z

python/lsst/ip/diffim/zogy.py

+
+        def gm(exp):
+            return exp.getMaskedImage().getMask()
+        def ga(exp):


line between functions (even inner functions).

yalsayyad · 2017-07-10T15:22:10Z

python/lsst/ip/diffim/zogy.py

+            return exp.getMaskedImage().getMask()
+        def ga(exp):
+            return exp.getMaskedImage().getImage().getArray()
+        def gv(exp):


gv is not used and is exactly the same as ga.

Also, user convenience functions like these make the code harder to read. Please don't do this.

Also, with pybind11, you can write this as e.g. exp.maskedImage.mask which might make this more palatable.

yalsayyad · 2017-07-10T16:23:33Z

python/lsst/ip/diffim/zogy.py

+        badBitsNan = mask.getPlaneBitMask(['UNMASKEDNAN'])
+        mask |= gm(scienceExposure)
+        mask |= gm(templateExposure)
+        gm(results.D)[:, :] = mask


mask here is a view (pointer) of results.D's mask (i.e. the two lines before have already modified it) This line is unnecessary as it just copies itself to itself.

yalsayyad · 2017-07-10T16:47:17Z

python/lsst/ip/diffim/zogy.py

@@ -478,7 +516,8 @@ def _setNewPsf(self, exposure, psfArr):
        exposure.setPsf(psfNew)
        return exposure

-    def computeDiffim(self, inImageSpace=None, padSize=None, **kwargs):
+    def computeDiffim(self, inImageSpace=None, padSize=None,
+                      returnMatchedTemplate=False, **kwargs):


Add returnMatchedTemplate to docstring.

yalsayyad · 2017-07-10T16:56:08Z

python/lsst/ip/diffim/zogy.py

+    def subtractMaskedImages(self, templateExposure, scienceExposure,
+                             doWarping=True, spatiallyVarying=True, inImageSpace=False,
+                             doPreConvolve=False):
+        pass  # not implemented


Consider if raise NotImplementedError would be better here.

- Include unit tests for new task Fix issues pointed out by reviewer Attempt to fix padding issues Ensure an UNMASKEDNAN mask Fix PSF padding - Add unit test for different-sized PSFs - Add some documentation - Other fixes from review Add subtractAlgorithmRegistry Various docstring fixes Remove `subTask` from class/config names

djreiss force-pushed the tickets/DM-10805 branch from ffa12aa to fe07bf8 Compare June 26, 2017 23:35

yalsayyad reviewed Jul 5, 2017

View reviewed changes

yalsayyad reviewed Jul 9, 2017

View reviewed changes

yalsayyad reviewed Jul 10, 2017

View reviewed changes

djreiss force-pushed the tickets/DM-10805 branch from 2708160 to cb7132d Compare August 9, 2017 01:21

djreiss force-pushed the tickets/DM-10805 branch from a6c487f to 9e9d795 Compare August 26, 2017 00:16

djreiss force-pushed the tickets/DM-10805 branch from 9e9d795 to 09d7660 Compare August 26, 2017 22:03

djreiss merged commit 2e13b0b into master Aug 28, 2017

ktlim deleted the tickets/DM-10805 branch August 25, 2018 06:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-10805: Spatially-varying ZOGY option #66

DM-10805: Spatially-varying ZOGY option #66

djreiss commented Jun 6, 2017

yalsayyad Jul 5, 2017 •

edited

djreiss Aug 9, 2017

yalsayyad Jul 5, 2017

PaulPrice Jul 5, 2017

djreiss Jul 7, 2017 •

edited

yalsayyad Jul 9, 2017

djreiss Aug 9, 2017

yalsayyad Jul 9, 2017

yalsayyad Jul 9, 2017 •

edited

yalsayyad Jul 9, 2017

djreiss Aug 9, 2017 •

edited

yalsayyad Jul 9, 2017

djreiss Aug 9, 2017

yalsayyad Jul 9, 2017 •

edited

yalsayyad Jul 10, 2017

yalsayyad Jul 10, 2017

RobertLuptonTheGood Jul 10, 2017

yalsayyad Jul 10, 2017

yalsayyad Jul 10, 2017

yalsayyad Jul 10, 2017

DM-10805: Spatially-varying ZOGY option #66

DM-10805: Spatially-varying ZOGY option #66

Conversation

djreiss commented Jun 6, 2017

yalsayyad Jul 5, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djreiss Jul 7, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yalsayyad Jul 9, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djreiss Aug 9, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yalsayyad Jul 9, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yalsayyad Jul 5, 2017 •

edited

djreiss Jul 7, 2017 •

edited

yalsayyad Jul 9, 2017 •

edited

djreiss Aug 9, 2017 •

edited

yalsayyad Jul 9, 2017 •

edited