DM-10803: Refactor grid generation in ImageMapReduceTask #64
Conversation
| if scaleByFwhm: | ||
| self.log.info("Scaling grid parameters by %f" % psfFwhm) | ||
| if cellCentroidsX is None or len(cellCentroidsX) <= 0: | ||
| # No given centroids; construct them from cellSize/gridStep |
|
|
||
| def rescaleValue(val): | ||
| psfFwhm = (exposure.getPsf().computeShape().getDeterminantRadius() * | ||
| 2. * np.sqrt(2. * np.log(2.))) |
There was a problem hiding this comment.
No spaces around multiplication.
| borderSizeX = rescaleValue(borderSizeX) | ||
| borderSizeY = rescaleValue(borderSizeY) | ||
|
|
||
| nGridX = bbox.getWidth() // gridStepX |
There was a problem hiding this comment.
No spaces around division (floor, or otherwise). Also below.
| # Readjust spacings so that they fit perfectly in the image. | ||
| nGridX = bbox.getWidth() // cellSizeX + 1 | ||
| nGridY = bbox.getHeight() // cellSizeY + 1 | ||
| cellCentroids = [(x, y) for x in np.linspace(cellSizeX//2, bbox.getWidth() - cellSizeX//2, |
There was a problem hiding this comment.
Double-nested list comprehensions are a bit unpleasant. I think what you want to do is use np.meshgrid here. Possibly:
np.meshgrid(np.linspace(...), np.linspace(...))See: https://docs.scipy.org/doc/numpy/reference/generated/numpy.meshgrid.html
There was a problem hiding this comment.
See my comment below for why.
| if bb.getWidth() % 2 == 1: # grow to the right | ||
| bb.include(afwGeom.Point2I(bb.getMaxX()+1, bb.getMaxY())) # Expand by 1 pixel! | ||
| bb.clip(bbox) | ||
| if bb.getWidth() % 2 == 1: # clipped at right -- so grow to the left |
There was a problem hiding this comment.
Could it ever be clipped on both sides, such that this this expansion also fails?
There was a problem hiding this comment.
Hmm, it's only possible if the cell size is > the image size. This shouldn't happen but I suppose if it does I can shrink the bb instead of growing it.
Ugh, Box2Ds are not very easily manipulatable. I'm just going to raise an exception if it is not even-sized after these two attempts, let's see how often it actually happens.
| # Use given or grid-parameterized centroids as centers for bounding boxes | ||
| if cellCentroidsX is not None and len(cellCentroidsX) > 0: | ||
| for i, centroidX in enumerate(cellCentroidsX): | ||
| centroidY = cellCentroidsY[i] | ||
| centroid = afwGeom.Point2D(centroidX, centroidY) |
There was a problem hiding this comment.
If you make a meshgrid above, you should be able to do:
for xy in cellCentroids.flatten():
centroid = afwGeom.Point2D(xy[0], xy[1])
...
There was a problem hiding this comment.
This doesn't work as meshgrid returns a list of arrays.
There was a problem hiding this comment.
Plus, the grid may not actually be a grid (if the config-ed cellCentroidsX and cellCentroidsY are not None). So I'm leaving as-is but refactoring a bit more to reduce code duplication.
- Rename 'gridCentroidsX/Y' to 'cellCentroidsX/Y' - Rename 'gridSizeX/Y' to 'cellSizeX/Y' - Refactor grid generation to iterate over centroids rather than offsetting bounding boxes
offsetting bounding boxes