Add CoaddPsf construction in default reducer subtask. #52
Conversation
isullivan
commented
Mar 28, 2017
isullivan
commented
- Add tests of newly-generated PSFs
- Fix a few docstrings
- Add some fixes that arose during testing
tests/testImageMapReduce.py
Outdated
| sumy2 += y*y*f | ||
| sumx += x*f | ||
| sumy += y*f | ||
| sum += f |
There was a problem hiding this comment.
How about sumf instead of sum, since sum is an existing function.
| @@ -523,7 +563,7 @@ def _reduceImage(self, mapperResults, exposure, **kwargs): | |||
| result = self.reducerSubtask.run(mapperResults, exposure, **kwargs) | |||
| return result | |||
|
|
|||
| def _generateGrid(self, exposure, forceEvenSized=False): | |||
| def _generateGrid(self, exposure, forceEvenSized=False, **kwargs): | |||
There was a problem hiding this comment.
Add a comment describing what you expect to pass through **kwargs
There was a problem hiding this comment.
That is noted in a comment where this function is called (line 528) but will add a comment here.
| it just copies over the PSF from the original exposure). To be | ||
| investigated in DM-9629. | ||
| 3. To be done: correct handling of masks as well. | ||
| 2. This correctly handles varying PSFs, constructing the resulting |
There was a problem hiding this comment.
- and 2. here aren't really 'known issues', since they're features. Maybe move them to a separate 'Features' section.
There was a problem hiding this comment.
Kept 1 and 2 in Notes, moved 3. and 4. to 'Known issues' section.
| @@ -230,14 +231,15 @@ def run(self, mapperResults, exposure, **kwargs): | |||
| newMI = newExp.getMaskedImage() | |||
|
|
|||
| reduceOp = self.config.reduceOperation | |||
| weights = None | |||
There was a problem hiding this comment.
Can you remove this? I don't see anywhere that it would be valid for weights to be None
There was a problem hiding this comment.
Need to set weights to None so that it is kept in function scope. Moved the setting to None to inside the first if clause -- when it won't be needed.
| subMI.getVariance().getArray()[isNotNan] += patchMI.getVariance().getArray()[isNotNan] | ||
| if reduceOp == 'average': | ||
| # wsubim is a view into the `weights` Image | ||
| wsubim = afwImage.ImageI(weights, item.getBBox()) |
There was a problem hiding this comment.
wsubim is not a very clear variable name. Maybe weightsView or wtSubView or something.
| a `subExposure` element, from which the component Psfs are | ||
| extracted (thus the reducerTask cannot have | ||
| `reduceOperation = 'none'`. | ||
| exposure : afwImage.Exposure |
There was a problem hiding this comment.
I suggest you write this as lsst.afw.image.Exposure, instead of using the shortened afwImage here, since it will show up in the documentation without the context of the import statement.
There was a problem hiding this comment.
The same comment applies to pipeBase above and measAlg below
There was a problem hiding this comment.
Will do in my commit to DM-7611, if that's okay (works off the same codebase and I don't want to add more conflicts).
| mycatalog = afwTable.ExposureCatalog(schema) | ||
|
|
||
| # We're just using the exposure's WCS (assuming that the subExposures' | ||
| # WCSs are the same, which they better be!). |
There was a problem hiding this comment.
Could you add a check and raise an exception if the WCS's were different?
There was a problem hiding this comment.
OK, but how to compare WCSs? Will investigate...
tests/testImageMapReduce.py
Outdated
| import lsst.pex.config as pexConfig | ||
| import lsst.meas.algorithms as measAlg | ||
| import lsst.pipe.base as pipeBase | ||
| import lsst.daf.base as dafBase |
There was a problem hiding this comment.
Please insert the new import statements in alphabetical order.
|
|
||
| if reduceOp == 'average': | ||
| newMI /= weights | ||
| wts = weights.getArray().astype(np.float) |
There was a problem hiding this comment.
I think you need to check for weights being zero anywhere.
There was a problem hiding this comment.
Good catch, I'm already working on setting a mask value to invalid in this case. This will be implemented as part of DM-7611 (about to submit for review).
- Add tests of newly-generated PSFs - Fix a few docstrings - Add some fixes that arose during testing
djreiss
force-pushed
the
tickets/DM-9629
branch
from
b4dcd55
to
b906e44
Compare
