DM-24325: Include CCDData cutouts in alert packets #89
Conversation
Debug new ccdData path. Debug unittests.
Debug unittest
There was a problem hiding this comment.
My only concern was what looks like a mismatch between crpix and crval in createCcdDataCutout, but maybe I am missing something. If the current implementation is correct, please add a comment to explain it.
I also made a couple comments about code in the file that was not touched by your changes. That could be outside the scope of this ticket, but if it's not too much trouble to address them please do so.
| @@ -106,6 +113,7 @@ def run(self, | |||
| self._patchDiaSources(diaSourceCat) | |||
There was a problem hiding this comment.
It's not part of your changes, but it doesn't look like template or ccdExposureIdBits are used anywhere.
There was a problem hiding this comment.
Eventually will so I'm going to leave it for now.
| diffIm.getCutout(sphPoint, cutoutBBox), | ||
| sphPoint, | ||
| diffImPhotoCalib) | ||
|
|
||
| templateCutout = None | ||
| # TODO: Create alertIds DM-24858 | ||
| alertId = diaSource["diaSourceId"] |
There was a problem hiding this comment.
Also not part of your changes, but I don't see how _patchDiaSources below changes the "currently grouped alert data to None" as the documentation says.
There was a problem hiding this comment.
Removed. Should have been removed in a previous ticket but was not.
| CCDData object storing the calibrate information from the input | ||
| difference or template image. | ||
| """ | ||
| cutOutMinX = cutout.getBBox().minX - 1 |
There was a problem hiding this comment.
Please add a comment explaining the additional -1 here.
| cutoutWcs = wcs.WCS(naxis=2) | ||
| cutoutWcs.array_shape = (cutout.getBBox().getWidth(), | ||
| cutout.getBBox().getWidth()) | ||
| cutoutWcs.wcs.crpix = [center.x - cutOutMinX, center.y - cutOutMinY] |
There was a problem hiding this comment.
Why is the crpix at the corner rather than the center of the cutout?
There was a problem hiding this comment.
So, CCDData doesn't store absolute pixel locations. So when you create a dataset with it the "bottom left" pixel is always [1, 1]. Thus we center the crpix such that it's correctly within this newly shifted cutout and at the location of the DiaSource in the cutout. There is a possible future feature in astropy to add an origin to the CCDData object but it is currently not a priority.
There was a problem hiding this comment.
I think "not a priority" is right for them but if we want to add that feature they seem like they would be more than happy to accept a patch. We have been in discussion for years to add pixel origin to NDData.
| cutout.getBBox().getWidth()) | ||
| cutoutWcs.wcs.crpix = [center.x - cutOutMinX, center.y - cutOutMinY] | ||
| cutoutWcs.wcs.crval = [sphPoint.getRa().asDegrees(), | ||
| sphPoint.getDec().asDegrees()] |
There was a problem hiding this comment.
Isn't this crval actually for the center of the bbox?
There was a problem hiding this comment.
It's the center of the DiaSource, see above.
| @@ -165,6 +177,80 @@ def createDiaSourceBBox(self, bboxSize): | |||
| bbox = geom.Extent2I(bboxSize, bboxSize) | |||
| return bbox | |||
|
|
|||
| def createCcdDataCutout(self, cutout, sphPoint, photoCalib): | |||
There was a problem hiding this comment.
I feel sphPoint describes the data type more than what the parameter is. Maybe something like centerPoint instead?
There was a problem hiding this comment.
Changed to skyCenter
| degrees. | ||
| """ | ||
| localGnomonicWcs = afwGeom.makeSkyWcs( | ||
| center, skyCenter, [[self._scale, 0], [0, self._scale]]) |
There was a problem hiding this comment.
I think this would be more readable if you first defined a variable such as cdMatrix = [[self._scale, 0], [0, self._scale]] and then used that here.
| The approximation is created as if the center is at RA=0, DEC=0. All | ||
| comparing x,y coordinate are relative to the position of center. Matrix | ||
| is initially calculated with units arcseconds and then converted to | ||
| radians. This yields higher precision results due to quirks in AST. |
There was a problem hiding this comment.
"converted to radians". Do you mean "converted to degrees"?
tests/test_packageAlerts.py
Outdated
| calibExposure = self.exposure.getPhotoCalib().calibrateImage( | ||
| self.exposure.getMaskedImage()) | ||
|
|
||
| self.assertTrue(np.allclose(ccdData.wcs.wcs.cd, |
There was a problem hiding this comment.
I think self.assertFloatsAlmostEqual(ccdData.wcs.wcs.cd, self.cutoutWcs.wcs.cd) might give more informative error messages if the test fails. The downside is you would have to have TestPackageAlerts inherit from lsst.utils.tests.TestCase instead of unittest.TestCase.
If you end up switching the assert here, also change the other self.assertTrue(np.allclose(... throughout this file.
morriscb
force-pushed
the
tickets/DM-24325
branch
from
58c8292
to
13cac58
Compare
Debug unittest Fix linting
morriscb
force-pushed
the
tickets/DM-24325
branch
from
13cac58
to
95ff93e
Compare
No description provided.