DM-38555: Implement BFE code improvements suggested by Lance Miller and Euclid colleagues #260
Conversation
There was a problem hiding this comment.
The majority of comments are about docstrings, and although they're not critical, it'd be good to get them right from the start.
One suggestion of variable renaming that confused me, but I think the algorithm is fine. After discussions earlier in the day, I think all of the indexing here is correct and consistent, with the convention that the input kernel here is already in the (y, x) orientation it should be for direct use in an afwImage.ExposureF.image.array.
tests/test_brighterFatter.py
Outdated
| @@ -85,6 +85,12 @@ def test_BrighterFatterInterface(self): | |||
| isrFunctions.brighterFatterCorrection(exp, kernelToUse, 5, 100, False) | |||
| self.assertImagesEqual(ref_image, image) | |||
|
|
|||
| isrFunctions.brighterFatterCorrection(exp, kernelToUse, 5, 100, False) | |||
There was a problem hiding this comment.
I think a copy/paste went wrong and put an additional copy of this test here.
python/lsst/ip/isr/isrFunctions.py
Outdated
| def fluxConservingBrighterFatterCorrection(exposure, kernel, maxIter, threshold, applyGain, | ||
| gains=None, correctionMode=True): | ||
| """Apply brighter fatter correction in place for the image. | ||
| Modified version conserves flux, with improved correction of PSF core. |
There was a problem hiding this comment.
I think this docstring needs to be reworded a bit. I'd move this line and the next to a new paragraph, switching "modified" for "this", such that
This version presents a modified version of the algorithm found in ``lsst.ip.isr.isrFunctions.brighterFatterCorrection`` which conserves the image flux, resulting in improved correction of the cores of stars. The convolution has also been modified to mitigate edge effects.
| This correction takes a kernel that has been derived from flat | ||
| field images to redistribute the charge. The gradient of the | ||
| kernel is the deflection field due to the accumulated charge. | ||
| Given the original image I(x) and the kernel K(x) we can compute |
There was a problem hiding this comment.
The original blank lines are required for this to format correctly in the auto-generated documentation. New lines before "Given", "Ic(x)", "Improved", "Because", and "Edges."
python/lsst/ip/isr/isrFunctions.py
Outdated
| a physical model for the edge, but avoids discontinuity in the correction. | ||
|
|
||
| Author of modified version: Lance.Miller@physics.ox.ac.uk | ||
| (see DM-33555). |
python/lsst/ip/isr/isrFunctions.py
Outdated
| Author of modified version: Lance.Miller@physics.ox.ac.uk | ||
| (see DM-33555). | ||
| """ | ||
|
|
There was a problem hiding this comment.
But this newline isn't allowed, and needs to be removed.
python/lsst/ip/isr/isrFunctions.py
Outdated
| # allows us to pad the image with zeros and avoid wrap-around effects | ||
| # (although still not handling the image edges with a physical model) | ||
| # This wouldn't be great if there were a strong image gradient. | ||
| imydim, imxdim = tempImage.array.shape |
There was a problem hiding this comment.
Can these be renamed to imYdim and imXdim? I'm fine with any other name, but without some separation, I read the first one as i-my-dim, which confused me.
python/lsst/ip/isr/isrFunctions.py
Outdated
| outArray = outImage.getArray() | ||
|
|
||
| # trim convolution output back to original shape | ||
| outArray = outArray[:imydim, :imxdim] |
There was a problem hiding this comment.
This needs to match above as well (I think this is the only place they're used).
No description provided.