Add imageMapReduce task #46

djreiss · 2017-02-23T17:07:28Z

Unit tests are in PR #45.

parejkoj

The overall design is reasonable. Several comments about docs, new unittests and returning Structs.

I think it would be good for you to run coverage (either via pytest-cov, or coverage.py), to help find where your current tests aren't getting to. I can help you with that, if you like.

Although I have some comments to that effect above, you do need some more docs about what happens when Map doesn't return an Exposure (e.g., moving average as you suggested), both at the file/class level, and for the method args/return values.

I do worry about about implementing MapReduce in the stack in ip_diffim, instead of either taking an off-the-shelf python MR code, or putting it somewhere higher (e.g. lsst.utils?). The implementation here is somewhat specific to your use case, though, so I don't know how feasible either of those would be. At the very least, you should think about how one would make this parallel via multiprocessing or mpi, since MR lends it self perfectly to that.

parejkoj · 2017-02-28T23:32:29Z

python/lsst/ip/diffim/imageMapReduce.py

+import lsst.pex.config as pexConfig
+import lsst.pipe.base as pipeBase
+
+__all__ = ("ImageMapReduceTask", "ImageMapReduceConfig",


__all__ should probably be at the very top. There was recently discussion of this, with this ticket filed: https://jira.lsstcorp.org/browse/DM-9596

It's good that you're using a tuple for these. Apparently there are "interesting" corner cases involving it being a list.

Thanks for pointing out, but I'll leave it as-is until there's an implemented RFC.

parejkoj · 2017-02-28T23:56:05Z

python/lsst/ip/diffim/imageMapReduce.py

+
+class ImageMapperSubtask(with_metaclass(abc.ABCMeta, pipeBase.Task)):
+    """Abstract base class for any task that is to be
+    used as `ImageMapReduceConfig.mapperSubtask`


This docstring could use some expansion: what would a non-abstract child class used by mapperSubtask actually do?

parejkoj · 2017-03-01T00:34:16Z

python/lsst/ip/diffim/imageMapReduce.py

+    _DefaultName = "ip_diffim_ImageMapperSubtask"
+
+    def run(self, subExp, expandedSubExp, fullBBox, **kwargs):
+        """Perform operation on given sub-exposure.


Perform what operation?

parejkoj · 2017-03-01T00:37:05Z

python/lsst/ip/diffim/imageMapReduce.py

+    ConfigClass = ImageMapperSubtaskConfig
+    _DefaultName = "ip_diffim_ImageMapperSubtask"
+
+    def run(self, subExp, expandedSubExp, fullBBox, **kwargs):


You need @abc.abstractmethod on here, and any other methods below that must be overridden in order to have a complete non-abstract child class.

parejkoj · 2017-03-01T00:37:49Z

python/lsst/ip/diffim/imageMapReduce.py

+        subExp : afw.Exposure
+            the sub-exposure upon which to operate
+        expandedSubExp : afw.Exposure
+            the expanded sub-exposure upon which to operate


What's an "expanded sub-exposure"?

parejkoj · 2017-03-01T02:05:49Z

python/lsst/ip/diffim/imageMapReduce.py

+        scaleByFwhm = self.config.scaleByFwhm
+        bbox = exposure.getBBox()
+
+        psfFwhm = (exposure.getPsf().computeShape().getDeterminantRadius() *


Technically, or coding standards don't like spaces around *, but I don't really care.

parejkoj · 2017-03-01T02:11:58Z

python/lsst/ip/diffim/imageMapReduce.py

+
+        def rescaleValue(val):
+            if scaleByFwhm:
+                return np.rint(val * psfFwhm).astype(int)


What happens if the psf has different dimensions in X vs. Y?

It uses the average.

parejkoj · 2017-03-01T02:14:51Z

python/lsst/ip/diffim/imageMapReduce.py

+            nGridX = bbox.getWidth() / gridStepX
+            # Readjust gridStepX so that it fits perfectly in the image.
+            gridStepX = float(bbox.getWidth() - gridSizeX) / float(nGridX)
+            if gridStepX <= 0:


<=0 here, or <=1? (same below)

This is just to catch a case which shouldn't happen. I'll raise an error instead.

parejkoj · 2017-03-01T02:15:42Z

python/lsst/ip/diffim/imageMapReduce.py

+                centroidY = self.config.gridCentroidsY[i]
+                centroid = afwGeom.Point2D(centroidX, centroidY)
+                bb0 = afwGeom.Box2I(bbox0)
+                xoff = int(np.floor(centroid.getX())) - bb0.getWidth()//2


Good use of explicit integer division.

parejkoj · 2017-03-01T02:18:25Z

python/lsst/ip/diffim/imageMapReduce.py

+            return bb0, bb1
+
+        xoff = 0
+        while(xoff <= bbox.getWidth()):


I've missed while loops!

djreiss · 2017-03-01T21:35:40Z

The overall design is reasonable. Several comments about docs, new unittests and returning Structs.

I think it would be good for you to run coverage (either via pytest-cov, or coverage.py), to help find where your current tests aren't getting to. I can help you with that, if you like.

Although I have some comments to that effect above, you do need some more docs about what happens when Map doesn't return an Exposure (e.g., moving average as you suggested), both at the file/class level, and for the method args/return values.

I do worry about about implementing MapReduce in the stack in ip_diffim, instead of either taking an off-the-shelf python MR code, or putting it somewhere higher (e.g. lsst.utils?). The implementation here is somewhat specific to your use case, though, so I don't know how feasible either of those would be. At the very least, you should think about how one would make this parallel via multiprocessing or mpi, since MR lends it self perfectly to that.

Thanks for the excellent review. I have no issues with nearly all of your comments. I agree with the potential for moving this out of ip_diffim if there is a use-case, since it is not specific to image differencing. For now, I have plans for two separate uses of ImageMapReduce in ip_diffim, which is why I placed it there.

parejkoj

Thanks for the updates. The docs are a lot clearer now.

See the one minor comment.

parejkoj · 2017-03-02T22:56:12Z

python/lsst/ip/diffim/imageMapReduce.py

@@ -175,7 +246,8 @@ class ImageMapReduceConfig(pexConfig.Config):
    )

    # Separate gridCentroidsX and gridCentroidsY since pexConfig.ListField accepts limited dtypes
-    #  (i.e., no Point2D)
+    #  (i.e., no Point2D). The resulting set of centroids is the "vertical stack" of
+    #  `gridCentroidsX` and `gridCentroidsY`.


Please give an example here, showing what you get for, e.g., (1,2 ), (3,4)

Add unit test

djreiss · 2017-03-08T00:03:33Z

Waited until pybind11 merge. Confirmed build on Jenkins.

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

parejkoj requested changes Mar 1, 2017

View reviewed changes

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 mentioned this pull request Mar 4, 2017

Add unit tests for ImageMapReduce task #45

Closed

djreiss force-pushed the tickets/DM-8520 branch 2 times, most recently from 2d253ea to 2f9cf10 Compare March 6, 2017 20:13

Add imageMapReduce task

ed01287

Add unit test

djreiss force-pushed the tickets/DM-8520 branch from 2f9cf10 to ed01287 Compare March 7, 2017 23:08

djreiss merged commit ab7f4cf into master Mar 8, 2017

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add imageMapReduce task #46

Add imageMapReduce task #46

djreiss commented Feb 23, 2017

parejkoj left a comment

parejkoj Feb 28, 2017

djreiss Mar 1, 2017

parejkoj Feb 28, 2017

parejkoj Mar 1, 2017

parejkoj Mar 1, 2017

parejkoj Mar 1, 2017

parejkoj Mar 1, 2017

parejkoj Mar 1, 2017

djreiss Mar 2, 2017

parejkoj Mar 1, 2017

djreiss Mar 2, 2017 •

edited

parejkoj Mar 1, 2017

parejkoj Mar 1, 2017

djreiss commented Mar 1, 2017

parejkoj left a comment

parejkoj Mar 2, 2017

djreiss commented Mar 8, 2017

Add imageMapReduce task #46

Add imageMapReduce task #46

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

Choose a reason for hiding this comment

djreiss Mar 2, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djreiss commented Mar 1, 2017

parejkoj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djreiss commented Mar 8, 2017

djreiss Mar 2, 2017 •

edited