DM-11951: Fixes in AL Decorrelation and Zogy #73

djreiss · 2017-09-19T23:36:36Z

No description provided.

r-owen

Looks good. A few minor suggestions

r-owen · 2017-09-19T23:50:13Z

python/lsst/ip/diffim/imageDecorrelation.py

@@ -164,6 +163,7 @@ def run(self, exposure, templateExposure, subtractedExposure, psfMatchingKernel,
        `ip_diffim.ImagePsfMatchTask.subtractExposures()`
        @param[in] psfMatchingKernel an (optionally spatially-varying) PSF matching kernel produced
        by `ip_diffim.ImagePsfMatchTask.subtractExposures()`
+        @param[in] if not None, then the `exposure` was pre-convolved with this kernel


This is missing the name of the parameter, e.g. you want @param[in] preConvKernel If not None.... Doxygen should have complained.

r-owen · 2017-09-19T23:55:33Z

python/lsst/ip/diffim/imageDecorrelation.py

@@ -198,14 +198,24 @@ def run(self, exposure, templateExposure, subtractedExposure, psfMatchingKernel,

        if svar is None:
            svar = self.computeVarianceMean(exposure)
+        if np.isnan(svar):  # Should not happen unless entire image has been masked.
+            svar = 1e-9


It worries me to do this silently. Assuming if it really only should happen if the entire image has been masked, I suggest in the if isnan(svar): branch that you check that the entire image has been masked and:

If the entire image has been masked, log a warning to that effect. Since this is a task it has a "log" attribute, so it is as simple as self.log.warn(...). I wonder if you could bail out early in that case?

If the entire image has not been masked then raise an exception, since that should not happen.
Also save that information (e.g. set a new variable entireImageMasked) so you can repeat the test on line 205 just below.

I understand your point. The cases where I have had this happen is when using the ImageMapReduce framework (which does these computations on a grid of subimages) when a subimage is covered by a mask. Now that you mention it, it can be short-circuited and just return a copy of the input subtractedExposure, which is all NaNs and/or masked. I'll do that.

r-owen · 2017-09-20T00:03:35Z

python/lsst/ip/diffim/imageDecorrelation.py

+        denom = svar + tvar * kft2
+        if preConvKernel is not None:
+            mk = DecorrelateALKernelTask._fixOddKernel(kft)
+            mk[mk < 1e-3] = 1e-3  # Psf should not be < 0; and messes up denominator


Please hard-coding the limit as a constant (e.g. double MIN_KERNEL = 1.0e-3) and document it where you define it. That keeps the code consistent in case you decide to change the limit -- the number only appears once.

Is 0.001 sufficiently small to be negligible? Some kernels can easily have 1000-ish pixels.

Given the code, I'm guessing kernel <= 0 messes up the denominator? Because if only negative values were a problem I would have expected the test to replace negative values with 0. If my supposition is correct, please update the comment, e.g. ...should not be <= 0....

All good points. Yes, it seems that 1e-3 is small enough -- smaller and strangeness can happen in the FFTs.

r-owen · 2017-09-20T00:04:30Z

python/lsst/ip/diffim/imageDecorrelation.py

+            mk = np.fft.fft2(mk)
+            mk2 = np.conj(mk) * mk
+            denom = svar * mk2 + tvar * kft2
+            denom[np.abs(denom) < 1e-3] = 1e-3  # fixes some aliasing issues


Is this 1e-3 intrinsically the same as the 1e-3 above for mk or an unrelated constant? If different, please define a new constant with documentation for why you chose the value.

r-owen · 2017-09-20T00:20:40Z

python/lsst/ip/diffim/imageDecorrelation.py

@@ -589,8 +609,8 @@ def run(self, scienceExposure, templateExposure, subtractedExposure, psfMatching
           by `ip_diffim.ImagePsfMatchTask.subtractExposures()`
        spatiallyVarying : bool
           if True, perform the spatially-varying operation
-        doPreConvolve : bool
-           if True, the scienceExposure has been pre-filtered with its PSF. (Currently
+        preConvKernel : bool


Please correct the type (it's not bool)

r-owen · 2017-09-20T00:21:31Z

python/lsst/ip/diffim/imageDecorrelation.py

@@ -601,7 +621,11 @@ def run(self, scienceExposure, templateExposure, subtractedExposure, psfMatching
        self.log.info('Running A&L decorrelation: spatiallyVarying=%r' % spatiallyVarying)

        svar = self.computeVarianceMean(scienceExposure)
+        if np.isnan(svar):  # Should not happen unless entire image has been masked.
+            svar = 1e-9


As before, please make sure the entire image really has been masked.

r-owen · 2017-09-20T00:22:52Z

python/lsst/ip/diffim/zogy.py

+            maxLoc2 = np.unravel_index(np.argmax(psf2), psf2.shape)
+
+        # Make sure there are no div-by-zeros
+        psf1[psf1 < 1e-4] = 1e-4


This is different than the 1e-3 you used in other code. Using a named constant would help, and it may be worth getting the constant that you use here from the value you set in the C++ code.

You're right, there's no logical reason for them to be different so I'll set them all to 1.0e-4. But there's not technically a reason for why they should be the same (the algorithms are different, they just suffer from related issues if the PSF is noisy).
Also I'm confused - there is no C++ code here, so can I set a global python variable and use it in both places?

Sorry, Python, not C++. Yes you could set one global python variable and use it in both places -- that makes sense if you can come up with a name that seems usable in the global API. If not, then a constant per module that is not exported would also be fine.

r-owen · 2017-09-20T00:23:50Z

python/lsst/ip/diffim/zogy.py

+        # Some masked regions are NaN or infinite!, and FFTs no likey.
+        def fix_nans(im):
+            """Replace any NaNs or Infs with the mean of the image."""
+            isbad = np.isinf(im) | np.isnan(im)


Consider isbad = not np.isfinite(im) as it handles a few more cases

Good advice! Should that be ~np.isfinite(im) (element-wise negation)?

Yes. Pity it looks like bitwise negation. np.logical_not(...) is more verbose but unambiguous. You decide.

r-owen · 2017-09-20T00:25:01Z

python/lsst/ip/diffim/imageDecorrelation.py

-        self.decorrelateMapReduceConfig.borderSizeX = self.decorrelateMapReduceConfig.borderSizeY = 6
+        self.decorrelateMapReduceConfig.gridStepX = self.decorrelateMapReduceConfig.gridStepY = 40
+        self.decorrelateMapReduceConfig.cellSizeX = self.decorrelateMapReduceConfig.cellSizeY = 41
+        self.decorrelateMapReduceConfig.borderSizeX = self.decorrelateMapReduceConfig.borderSizeY = 8


This code appears to be repeated below (line 1042). Can it be centralized?

I think you mean it's also in the zogy.py code. I can centralize it as the default in the imageMapReduce.ImageMapReduceConfig. However, there might be a reason to have these be different in the future. Right now I'm just finding that the same values work for both tasks. Do you have a suggestion about where to centralize this?

It's a good question. It looks like the defaults here actually belong to the config self.decorrelateMapReduceConfig, which is a DecorrelateALKernelMapReduceConfig, rather than this config. If so, and if that is true for zogy.py then I suggest those two map-reduce config classes inherit from a new common base class (e.g. BaseMapReduceConfig) that defines these common fields and assigns suitable defaults. Then in the future if one wishes to depart from any of these defaults, one can do that in the appropriate subclass or here.

For this to work setDefaults will have to call setDefaults on contained configs -- which is a good thing to do in any case.

However, such a change could be done as part of the refactoring. It's just the since you had to change the values in 2 places, that's a red flag.

r-owen · 2017-09-20T02:59:06Z

python/lsst/ip/diffim/imageDecorrelation.py

@@ -24,7 +24,6 @@
 #

 import numpy as np
-import scipy.fftpack


Please also remove the lines

from future import standard_library standard_library.install_aliases()

from near the top of the file -- these are no longer needed and make a hash of flake8 linting.

r-owen approved these changes Sep 20, 2017

View reviewed changes

r-owen reviewed Sep 20, 2017

View reviewed changes

Fix known issues with Zogy, decorrelation

a05ce55

djreiss force-pushed the tickets/DM-11951 branch from 53304cd to a05ce55 Compare September 20, 2017 22:44

djreiss merged commit 01682fb into master Sep 20, 2017

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-11951: Fixes in AL Decorrelation and Zogy #73

DM-11951: Fixes in AL Decorrelation and Zogy #73

djreiss commented Sep 19, 2017

r-owen left a comment

r-owen Sep 19, 2017

r-owen Sep 19, 2017

djreiss Sep 20, 2017

r-owen Sep 20, 2017

djreiss Sep 20, 2017

r-owen Sep 20, 2017

r-owen Sep 20, 2017

r-owen Sep 20, 2017

r-owen Sep 20, 2017

djreiss Sep 20, 2017

r-owen Sep 20, 2017

r-owen Sep 20, 2017

djreiss Sep 20, 2017

r-owen Sep 20, 2017

r-owen Sep 20, 2017

djreiss Sep 20, 2017

r-owen Sep 20, 2017

r-owen Sep 20, 2017

DM-11951: Fixes in AL Decorrelation and Zogy #73

DM-11951: Fixes in AL Decorrelation and Zogy #73

Conversation

djreiss commented Sep 19, 2017

r-owen left a 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

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

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