DM-19802: Fix jointcal ra/dec bounding box calculations #142
Conversation
In addition to being useful for this test, this is a function we may want in a future "parallelize data reads" approach.
This lets us specify new on-sky positions for the fake catalogs.
parejkoj
force-pushed
the
tickets/DM-19802
branch
from
d8bbf6e
to
028046c
Compare
| AstrometryTransformLinear identity; | ||
| TanPixelToRaDec commonTangentPlaneToRaDec(identity, _commonTangentPoint); | ||
| Frame raDecFrame = commonTangentPlaneToRaDec.apply(tangentPlaneFrame, false); | ||
|
|
There was a problem hiding this comment.
I'll have to trust based on the tests that this works, because I have no experience with the wcs Frames.
There was a problem hiding this comment.
The Frame in this case, is actually a jointcal object. I want to replace it with lsst.geom.Box2D at some point (DM-4044).
| points.emplace_back(sphgeom::LonLat::fromDegrees(raDecFrame.xMax, raDecFrame.yMin)); | ||
| points.emplace_back(sphgeom::LonLat::fromDegrees(raDecFrame.xMin, raDecFrame.yMax)); | ||
| points.emplace_back(sphgeom::LonLat::fromDegrees(raDecFrame.xMax, raDecFrame.yMax)); | ||
|
|
There was a problem hiding this comment.
What confused me here is that I guess x and y in the raDecFrame are in sky coordinates? It is not apparent from the x and y terminology (outside the scope of this ticket!) but maybe it's worth adding a comment to this effect? (Or it might be well-known to those more familiar with how the wcs Frames work.)
There was a problem hiding this comment.
Yes, this is confusing because jointcal's internal objects (Frame and Point) are used in both on-sky, and on-sensor cases, with x and y having different meanings in those cases. This is unfortunate, but I haven't had time to do the plumbing work to fix it (again, DM-4044).
I've added a comment that may clarify things?
tests/test_jointcal.py
Outdated
| import lsst.geom | ||
| from lsst.meas.algorithms import getRefFluxField, LoadIndexedReferenceObjectsTask, DatasetConfig | ||
| import lsst.pipe.base | ||
| import lsst.jointcal | ||
| from lsst.jointcal import MinimizeResult | ||
| # from lsst.jointcal.jointcal import computeBoundingCircle |
tests/test_jointcal.py
Outdated
| for x in bbox.getCorners()] | ||
| points.extend([lsst.geom.SpherePoint(wcs2.pixelToSky(lsst.geom.Point2D(x))) | ||
| for x in bbox.getCorners()]) | ||
| self._checkPointsInCircle(points, center, radius) |
There was a problem hiding this comment.
I can't help but notice that lines 327-341 are identical to 290-304, as best as I can tell. Is there a way to consolidate this?
|
I've pushed an update that responds to your comments. Is the new docstring better? |
parejkoj
force-pushed
the
tickets/DM-19802
branch
from
25cb324
to
16f27c6
Compare
Use sphgeom and SpherePoint to correctly compute the "bounding circle" of all ccdImages. The old code was wildly wrong near RA=0, and also sometimes had the wrong radius.
Now that the center and radius math is fixed, some of the numbers of loaded reference sources are slightly different (and more correct!).
parejkoj
force-pushed
the
tickets/DM-19802
branch
from
b02d06e
to
43ed731
Compare
This should fix the problem, and includes a test that demonstrates the fix.