fix variance calculation to use masks #26

djreiss · 2016-07-21T23:07:23Z

fix normalization of correction kernel
remove commented print statements
add coordinate parameters to run for testing
update test for stricter tolerance after bug fix
turn on old ClassificationDipole again
add options to DecorrelateALKerneltask.run()
change all sig**2 to variance
Fix mixup of var1 and var2 for science and template

parejkoj · 2016-07-22T19:48:25Z

python/lsst/ip/diffim/imageDecorrelation.py

@@ -130,19 +129,28 @@ def __init__(self, *args, **kwargs):
                                                                     "NO_DATA", "DETECTED_NEGATIVE"]))

    @pipeBase.timeMethod
-    def run(self, templateExposure, exposure, subtractedExposure, psfMatchingKernel):
+    def run(self, exposure, templateExposure, subtractedExposure, psfMatchingKernel,
+            xcen=None, ycen=None, var1=None, var2=None):
        """! Perform decorrelation of an image difference exposure.


Might be handy to note in the docstring that exposure, template match equation whatever in some paper.

It's not in a paper, yet. But I will reference the (still in progress) DMTN-021 with those details.

I see the reference. Thanks. Maybe I_1 and I_2 should be I_s and I_t in DMTN-021, to make it explicit?

parejkoj · 2016-07-23T00:12:35Z

tests/testImageDecorrelation.py

-        (to within a 1% tolerance).
-        """
-
+    def _testImages(self):
        # Create the matching kernel. We used Gaussian PSFs for im1 and im2, so we can compute the "expected"


Though it's a "private" method, it would be good to have a short docstring here that says what gets tested.

parejkoj · 2016-07-23T00:21:54Z

Few things, besides the specific comments I had on lines (some of which are expanded on below):

You can replace your singleGaussian2d() with astropy.modeling's Gaussian2D as follows (check that it doesn't break anything). Plus, since it's an object, you can instantiate it once and just call it in the loop as needed, which will make the intention of that loop clearer.

x,y = np.meshgrid(np.linspace(blah), np.linspace(blah))
someGaussian2d = astropy.modeling.functional_models.Gaussian2D(1,0,2,1,2,np.pi/4)
someGaussian2d(x,y)

Please double-check your linter config: a few off-standard lines have slipped through (not necessarily in the reviewed lines).
If you pull those staticmethods out to top level functions, you can then easily write a unittest for each, to help ensure they're e.g. padding how you want them to.
I probably shouldn't be surprised that afw.math.makeStatistics doesn't have a non-verbose way to get a variance, mean, etc. from a maskedImage. Given that, you can cut out a lot of code duplication by writing a top level imageDecorrelation functions to return the clipped-mean variance of a MaskedImage, and maybe another for one of the other statistics you're always computing.
Your tests would be more robust if you better quantified what the test was (e.g. var-expected_var > 10 or var-expected_var < 10), since assertNotClose covers quite a broad range of possibilities.

parejkoj · 2016-07-23T00:31:25Z

tests/testImageDecorrelation.py

+        (to within a 1% tolerance).
+        """
+        self._setUpImages(var1=0.04, var2=0.08)
+        self._testImages()


 def suite():


TestSuite is dead: long live py.test! See SQR-012 and the developer docs for more details:

https://sqr-012.lsst.io/
https://developer.lsst.io/coding/python_testing.html

OK, converted.

fix normalization of correction kernel remove commented print statements add coordinate parameters to `run` for testing update test for stricter tolerance after bug fix turn on old ClassificationDipole again add options to DecorrelateALKerneltask.run() change all sig**2 to variance Fix mixup of var1 and var2 for science and template Rename var1 to svar and var2 to tvar Improvements to unit tests Other improvements thanks to code review

parejkoj reviewed Jul 22, 2016
View reviewed changes

parejkoj reviewed Jul 23, 2016
View reviewed changes

djreiss force-pushed the tickets/DM-6580 branch from 64895dd to c99ac41 Compare July 26, 2016 01:11

djreiss merged commit 840e2db into master Jul 26, 2016

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix variance calculation to use masks #26

fix variance calculation to use masks #26

djreiss commented Jul 21, 2016

parejkoj Jul 22, 2016

djreiss Jul 25, 2016

parejkoj Jul 25, 2016

parejkoj Jul 23, 2016

djreiss Jul 25, 2016

parejkoj commented Jul 23, 2016

parejkoj Jul 23, 2016

djreiss Jul 25, 2016

parejkoj Jul 26, 2016

fix variance calculation to use masks #26

fix variance calculation to use masks #26

Conversation

djreiss commented Jul 21, 2016

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

parejkoj commented Jul 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment