Add unit tests for ImageMapReduce task #45

djreiss · 2017-02-23T17:04:43Z

No description provided.

parejkoj

Several changes requested in comments. I haven't looked at the actual functional code that this is testing, but this test structure makes sense to me. I do feel like there should be at least one more test that does something a bit more complicated: for example, although you set the PSF of the exposure, it doesn't look like it actually impacts the results of any of the tests.

parejkoj · 2017-02-24T05:58:24Z

tests/testImageMapReduce.py

@@ -0,0 +1,243 @@
+from __future__ import absolute_import, division, print_function
+from builtins import range
+from past.builtins import basestring


Do you need the basestring import here? I don't see it used.

parejkoj · 2017-02-24T06:07:56Z

tests/testImageMapReduce.py

+
+
+class TestImageMapperSubtaskConfig(ImageMapperSubtaskConfig):
+    """!


Please switch to pydoc style for all python docstrings here, and below: https://developer.lsst.io/docs/py_docs.html

parejkoj · 2017-02-24T06:11:11Z

tests/testImageMapReduce.py

+    _DefaultName = "ip_diffim_TestImageMapperSubtask"
+
+    def __init__(self, *args, **kwargs):
+        """! Create the image gridding subTask


The docstring for __init__ goes in the class-level docstring.

parejkoj · 2017-02-24T06:11:58Z

tests/testImageMapReduce.py

+        @param *args arguments to be passed to lsst.pipe.base.task.Task.__init__
+        @param **kwargs keyword arguments to be passed to lsst.pipe.base.task.Task.__init__
+        """
+        ImageMapperSubtask.__init__(self, *args, **kwargs)


I don't think you even need this __init__, if you're not doing anything besides calling super.

parejkoj · 2017-02-24T06:26:00Z

tests/testImageMapReduce.py

+        self.exposure = afwImage.ExposureF(128, 128)
+        self.exposure.setPsf(measAlg.DoubleGaussianPsf(11, 11, 2.0, 3.7))
+        mi = self.exposure.getMaskedImage()
+        mi[:, :] = 0.


Huh. I just looked, and didn't see an Exposure constructor that would assign a value to the entire maskedImage. Could you ask slack#dm if such a constructor exists, and if not file a ticket against afw?

For exposures, that would be tricky (image, variance, masks, psfs, etc.) To me, that would only make sense if it explicitly sets everything to zero. I am not sure if the default empty constructor guarantees that (probably not, for speed), so I just do it explicitly here.

It's good we brought this up: https://jira.lsstcorp.org/browse/DM-9593

parejkoj · 2017-02-24T06:48:33Z

tests/testImageMapReduce.py

+
+                    mn = newArr.mean()
+                    self.assertClose(mi.getImage().getArray().mean(), mn - expectedVal)
+                    self.assertClose(mn, expectedVal)


Same comments as above about testing the full array.

parejkoj · 2017-02-24T06:49:35Z

tests/testImageMapReduce.py

+
+                    task = ImageMapReduceTask(config)
+                    boxes = task._generateGrid(self.exposure)
+                    if len(boxes[0]) > 1000:  # bypass to prevent slow testing


How much slower is it? What bit of code makes it slow? Seems like a potentially interesting possible failure mode (having many boxes).

It is not a failure; it would just lead to a very long unit test run which I figure we want to avoid for regular CI. As it turns out I adjusted the parameters to be more realistic and now this case never happens. But I will leave it in there just in case.

I'm uncomfortable with a "just in case" thing that disables tests. If you've tweaked the parameters so that it doesn't happen, just get rid of it. If a future person re-changes the parameters such that the test takes a long time, they can figure out what to do then.

parejkoj · 2017-02-24T06:50:44Z

tests/testImageMapReduce.py

+        for scaleByFwhm in (False, True):
+            config.scaleByFwhm = scaleByFwhm
+            for gsx in range(51, 1, -10):
+                config.gridStepX = config.gridSizeX = gsx


Should the gridSize and gridStep always be equal? If not, you should have at least one test where they're different.

parejkoj · 2017-02-24T06:52:59Z

tests/testImageMapReduce.py

+        expectedVal = 1.
+        config.mapperSubtask.addAmount = expectedVal
+
+        for scaleByFwhm in (False, True):


I'd put the contents of one of these loops (maybe the inner-most one?) in a separate method, say _testGridValidity(self, scaleByFwhm, gridX, gridY), and then call that. The nested loops are a bit unwieldy.

It also concerns me that scaleByFwhm doesn't change any of the results. You should probably have a test where that value matters, otherwise it might be doing nothing and you'd never know.

scaleByFwhm changes the grid parameters, but here we simply want to ensure that given a set of input grid parameters, we don't fail and the final output image is not wrong. Thus the fact that they are the same is a good thing. That said, you're right that we should check the grid itself. I'll add a test for that.

parejkoj · 2017-02-24T06:55:07Z

tests/testImageMapReduce.py

+    """
+    addAmount = pexConfig.Field(
+        dtype=float,
+        doc="""Amount to add to image""",


Don't need triple quotes here.

parejkoj

More comments on the latest commit.

parejkoj · 2017-02-27T23:44:51Z

tests/testImageMapReduce.py

+            Bounding box of original exposure (not used here)
+        kwargs
+            Arbitrary keyword arguments (ignored)
+        -------


Don't need line here, just under Returns.

parejkoj · 2017-02-27T23:46:31Z

tests/testImageMapReduce.py

+            Bounding box of original exposure (not used here)
+        kwargs
+            Arbitrary keyword arguments (ignored)
+        -------


Another unnecessary line.

parejkoj · 2017-02-28T00:29:43Z

tests/testImageMapReduce.py

+            boxes = task._generateGrid(self.exposure)
+            ind = 0 if scaleByFwhm else 1
+            lenBoxes[ind] = len(boxes[0])
+            if len(boxes[0]) > 1000:  # bypass to prevent slow testing


I still don't like needing this if statement, particularly if you've reworked it to not result in such a large number of boxes.

parejkoj · 2017-02-28T00:30:40Z

tests/testImageMapReduce.py

-                    mn = newArr.mean()
-                    self.assertClose(mi.getImage().getArray().mean(), mn - expectedVal)
-                    self.assertClose(mn, expectedVal)
+            if scaleByFwhm:


This suggests to me that you should have two separate test methods, one for scaleByFwhm=True and the other for False.

That will lead to a lot of code duplication. Plus I want to compare the results of scaleByFwhm=True vs. False.

parejkoj · 2017-02-28T00:32:38Z

tests/testImageMapReduce.py


    def testGridValidity(self):
-        """!Test sample grids with various spacings and sizes and other options.
+        """Test sample grids with various spacings and sizes and other options.

        Try to see if we can break it.


I'm not sure this line of the docstring is actually helpful? Maybe if you defined what this could break, and how?

parejkoj

Thanks for the added tests. I'll bet the coverage is a fair bit better now.

A few comments about preferred assert choices.

parejkoj · 2017-03-02T23:15:19Z

tests/testImageMapReduce.py

-        mi = self.exposure.getMaskedImage()
+        isnan = np.isnan(newArr)
+        if not withNaNs:
+            self.assertTrue(np.sum(isnan) == 0,


Don't you want self.assertEqual(np.sum(isnan), 0)?

parejkoj · 2017-03-02T23:15:53Z

tests/testImageMapReduce.py

@@ -130,58 +142,122 @@ class ImageMapReduceTest(lsst.utils.tests.TestCase):
    """A test case for the image gridded processing task
    """
    def setUp(self):
+        self._makeImage()


Since you're using msg= now, probably set self.longMessage=True, to get both your custom messages, and the default unittest ones.

Apparently True is the default, but I'll set it explicitly anyway.

parejkoj · 2017-03-02T23:16:51Z

tests/testImageMapReduce.py

+        self.testAverageVersusCopy(withNaNs=False)
+        self.testAverageVersusCopy(withNaNs=True)
+
+    def testAverageVersusCopy(self, withNaNs=False):


This should have a preceeding _, otherwise it'll get run by the test suite on its own.

Oops, missed that.

parejkoj · 2017-03-02T23:17:17Z

tests/testImageMapReduce.py

        newMI = newExp.getMaskedImage()
        newArr = newMI.getImage().getArray()
        mi = self.exposure.getMaskedImage()
+        isnan = np.isnan(newArr)
+        if not withNaNs:
+            self.assertTrue(np.sum(isnan) == 0,


Another assertEqual here.

parejkoj · 2017-03-02T23:18:20Z

tests/testImageMapReduce.py

+        newMA1 = newMI1.getImage().getArray()
+        isnan = np.isnan(newMA1)
+        if not withNaNs:
+            self.assertTrue(np.sum(isnan) == 0)


assertEqual

parejkoj · 2017-03-02T23:18:39Z

tests/testImageMapReduce.py

+            self.assertTrue(np.sum(isnan) == 0)
+        newMA2 = newMI2.getImage().getArray()
+
+        self.assertFloatsAlmostEqual(newMA1[~isnan], newMA2[~isnan])


parejkoj · 2017-03-02T23:20:14Z

tests/testImageMapReduce.py

            newMI = newExp.getMaskedImage()
            newArr = newMI.getImage().getArray()
-            mi = self.exposure.getMaskedImage()
+            isnan = np.isnan(newArr)
+            self.assertTrue(np.sum(isnan) == 0, msg='Failed NaN (%d), on config: %s' %


assertEqual again

parejkoj · 2017-03-02T23:20:36Z

tests/testImageMapReduce.py

                                         msg='Failed on config: %s' % str(config))

-        self.assertTrue(lenBoxes[0] < lenBoxes[1], msg='Failed on config: %s' % str(config))
+        self.assertTrue(lenBoxes[0] < lenBoxes[1], msg='Failed lengths on config: %s' %


self.assertLess(lenBoxes[0], lenBoxes[1], ...)

djreiss · 2017-03-04T01:44:51Z

Final commit of the unit test in this PR was instead moved to PR #46, to be merged alongside ImageMapReduceTask.

djreiss added 2 commits February 22, 2017 15:15

Add imageMapReduce task

2a54b19

Add unit tests for ImageMapReduce task

6daf8e2

djreiss mentioned this pull request Feb 23, 2017

Add imageMapReduce task #46

Merged

parejkoj requested changes Feb 24, 2017

View reviewed changes

Edits to address reviewer concerns

85e5917

parejkoj reviewed Feb 28, 2017

View reviewed changes

djreiss force-pushed the tickets/DM-8520 branch from ce87058 to 31b7b1b Compare February 28, 2017 20:47

Update ImageMapReduce unit tests based on review

1d79b53

parejkoj approved these changes Mar 2, 2017

View reviewed changes

djreiss force-pushed the tickets/DM-8520 branch from 32d9686 to 970e71e Compare March 3, 2017 21:32

djreiss closed this Mar 4, 2017



		class TestImageMapperSubtaskConfig(ImageMapperSubtaskConfig):
		"""!

Add unit tests for ImageMapReduce task #45

Add unit tests for ImageMapReduce task #45

Conversation

djreiss commented Feb 23, 2017

parejkoj 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

djreiss Feb 24, 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

parejkoj 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

parejkoj 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

djreiss commented Mar 4, 2017

djreiss Feb 24, 2017 •

edited