DM-24371 Implement fixed correction fixed PSF support decorrelation afterburner #154
Conversation
kgabor
force-pushed
the
tickets/DM-24371
branch
from
155e5da
to
547ec60
Compare
There was a problem hiding this comment.
Looks good. Please address the individual comments below, but note that a few came up multiple times:
- You should make use of exposure attributes instead of
getin many places. E.g. replace instances ofexposure.getMaskedImage().getVariance().getArray()withexposure.variance.array. - When you return a
Structwith only one element, you should think about whether you really want theStructat all. - Watch out for reusing the same variable for different purposes within a method, that could easily lead to bugs or at least confusion later on.
| result : `Struct` | ||
| a `lsst.pipe.base.Struct` containing: | ||
|
|
||
| result : `lsst.pipe.base.Struct` |
There was a problem hiding this comment.
Since you're now only returning correctedExposure, do you still need to wrap it in a Struct?
There was a problem hiding this comment.
This is a result of dropping the correction kernel from the return. I am not 100% sure that we don't want to return any additional information; I'll postpone the removal of the Struct for the spatially varying correction implementation DM-23857. Add TODO comment to the task description and to the return statement.
Considerations: It'd be logical to return something about the correction, but...
- returning the Fourier space correction needs the freqSpaceShape and may not suit for any outside operation.
- Returning the image space correction (full image size) needs one additional, otherwise unnecessary, IFT. Also where the origin is placed would need detailed description or additional, otherwise unnecessary, copying (re-centering) of the array.
- If a matchingKernel size cropped version is returned then we throw away the main lesson from this whole change i.e. that the correction can be larger than the original matching kernel in general so should not be used.
| return pipeBase.Struct(correctedExposure=outExposure, ) | ||
|
|
||
| tOverSVar = tvar/svar | ||
| if tOverSVar > 1e8: |
There was a problem hiding this comment.
Please add a comment describing why 1e8 was chosen. Also, since this is only a warning, you might want to consider lowering the threshold. Finally, I'm a bit surprised by this check: isn't the template variance usually smaller than the science image, rather than the other way around?
There was a problem hiding this comment.
As the matching kernel high frequency components go to zero, the correction factor converges to sqrt(tvar/svar) i.e. this is a warning when high frequency pixels of the difference image is multiplied by more than 4 orders of magnitude as correction. Yes, in the regular case, the template variance is the smaller one and this is no concern; tvar>svar can happen if we convolve the science image thus the arguments are swapped for this task.
I don't have a really good justification for the value itself at the moment, just wanted an indicator to watch for diverging solutions. We won't coadd ~1e8 images, so I don't expect that any coadd variance will be this small (i.e. this large when swapped in science exposure convolution mode) but this scaling is still acceptable at double precision.
kgabor
force-pushed
the
tickets/DM-24371
branch
2 times, most recently
from
5288283
to
f8ce90b
Compare
* Add padding methods for DFT. * Update docstrings, fix shape bug. * Disable ImageMapReduce testing. * Add direct correlation correction unit test. * Add sourceless test image generation modification to `makeFakeImages`. * Add direct correlation estimation function `estimatePixelCorrelation`. * Add new test case `testNoiseDiffimCorrection`. * Update docstrings, improve code readability.
kgabor
force-pushed
the
tickets/DM-24371
branch
from
f8ce90b
to
971986d
Compare
No description provided.