DM-43515: Evaluate PSFs at the center of cells #48
Conversation
arunkannawadi
force-pushed
the
tickets/DM-43515
branch
2 times, most recently
from
cb3d87d
to
5c057be
Compare
| dtype=CoaddPsfConfig, | ||
| psf_warper = ConfigField( | ||
| doc="Configuration for the warper that warps the PSFs. It must have the same configuration used to " | ||
| "warp the images.", |
There was a problem hiding this comment.
We will have to enforce this via a contract in the pipeline, since I don't think there's any other way to enforce consistency across tasks.
There was a problem hiding this comment.
I agree that's the best approach for now, but I think it's also a nudge in the direction of moving PSF warping down to MakeDirectWarpTask (by having that warp yet another image plane that would for now have to be yet another separate dataset type). I don't think we should consider doing that on this ticket, but between configuration consistency, free parallelism over visits, and ruling out differences in how the WCS is interpolated during warping, I think that's the direction we want to go eventually.
There was a problem hiding this comment.
I see that to be the natural progression, given that we've moved PSF evaluation from measurement to coaddition and the next natural step is to move it to make_direct_warp.py. We spoke about this, but my thought here has been to use MultipleCellCoadd (we might need to subclass it, more on that later) with no buffer between cells to be the data structure to hold warps. It has the capability to hold multiple noise image planes which are used by cell-based coadds as well. The PSF images can be evaluated at the cell centers at the time of warping.
The reason why we might need another class - or make a generic parent class to subclass from - is that we now have to allow for multiple detectors within each cell (we don't want to discard them just yet). This needs some minor changes to how we serialize metadata info.
But to reiterate again,
differences in how the WCS is interpolated during warping
is not an issue because we evaluate the PSF image on the calexp and warp them here.
There was a problem hiding this comment.
But to reiterate again,
differences in how the WCS is interpolated during warping
is not an issue because we evaluate the PSF image on the calexp and warp them here.
We don't evaluate the PSF image on the full calexp; we evaluate it into a much smaller image in calexp coordinates. That would be sufficient if warping used the real WCS for every pixel, or if it evaluated the WCS at fixed points in PARENT coordinates (accounting for xy0) and interpolated between them. But I suspect that it might actually evaluate the WCS at fixed points in LOCAL coordinates (i.e. not accounting for xy0) and interpolate those. Just suspicion, though; if you know that it evaluates the PSF at fixed points in PARENT coordinates then this concern is indeed unfounded.
There was a problem hiding this comment.
Isn't this related to your comment about xy0 below? The bounding box of the PSF image is evaluated in PARENT coordinates by default but I wonder if I'm missing your point. Happy to discuss this more tomorrow.
There was a problem hiding this comment.
This question doesn't need to hold up this branch anyway, and I think the answer is one that can be found in the depths of the warping code: we're making different assumptions about what it's doing, but the truth is out there.
| dtype=CoaddPsfConfig, | ||
| psf_warper = ConfigField( | ||
| doc="Configuration for the warper that warps the PSFs. It must have the same configuration used to " | ||
| "warp the images.", |
There was a problem hiding this comment.
I agree that's the best approach for now, but I think it's also a nudge in the direction of moving PSF warping down to MakeDirectWarpTask (by having that warp yet another image plane that would for now have to be yet another separate dataset type). I don't think we should consider doing that on this ticket, but between configuration consistency, free parallelism over visits, and ruling out differences in how the WCS is interpolated during warping, I think that's the direction we want to go eventually.
|
|
||
| # Convert the PSF image from Image to MaskedImage. | ||
| undistorted_psf_maskedImage = afwImage.MaskedImageD(image=undistorted_psf_im) | ||
| # TODO: In DM-43585, use the variance plane value from noise. |
There was a problem hiding this comment.
I don't think it matters; the stacker had better not be using this variance for anything other than the output variance (we're passing it an explicit weight below), and we're throwing the output variance away.
There was a problem hiding this comment.
I have to double check, but we need that variance because its inverse is used as the weight.
| # at the closest integer location to the requested location. | ||
| undistorted_psf_im.setXY0( | ||
| geom.Box2I.makeCenteredBox(calexp_point, undistorted_psf_im.getDimensions()).getMin() | ||
| ) |
There was a problem hiding this comment.
I don't think we want this setXY0; Psf.computeImage should already be setting xy0 such that the PSF image we've just rendered looks like a star at the right position, and that's what the warpImage call below expects. I don't understand why it isn't causing trouble in your testing, though, unless it turns out to be a no-op or near no-op.
There was a problem hiding this comment.
It turned out to be a no-op indeed, since the bbox I am setting is the bbox that it already has.
arunkannawadi
force-pushed
the
tickets/DM-43515
branch
from
5c057be
to
fe0d7d3
Compare
This must not exclude stacking on masks, since they have already been interpolated.
arunkannawadi
force-pushed
the
tickets/DM-43515
branch
from
fe0d7d3
to
1747732
Compare
No description provided.