DM-14932: Add utility functions for creating SkyWcss from boresight/rotator + cameraGeom #370
Conversation
orientation is of pixels, not focal plane
r-owen
force-pushed
the
tickets/DM-14932
branch
2 times, most recently
from
9ca98ab
to
fb6f4ce
Compare
There was a problem hiding this comment.
Looks good. I really appreciate the care taken to document the sign and orientation conventions.
My only structural concern is that the tests feel a bit circular, in the sense that they delegate to much of the same code as the implementation and hence there isn't an external check on e.g. the sign correctness of flips. They're not entirely circular, by an means, and they're certainly still useful - but I'd have liked to see a test code path that took a point,
- transformed it via a PIXELS->FIELD_ANGLE transform,
- then transformed it via an AffineTransform (or equivalent) generated from
crval,orientation, andflipX(ideally by composing those directly, not with a call tomakeCdMatrix, though maybe I'm being paranoid), - then compared that to the same point run through
SkyWcs.pixelToSky
(and/or the reverse, i.e.skyToPixel).
EDIT: I just realized my test proposal is ill-formed, as it doesn't include the actual projection anywhere. Anyhow, maybe I'm being paranoid, so I'd certainly accept an argument for why these tests are sufficient in lieu of adding to them, especially since I apparently don't actually have a concrete proposal for how to do better.
include/lsst/afw/geom/SkyWcs.h
Outdated
| std::shared_ptr<SkyWcs> makeSkyWcs(TransformPoint2ToPoint2 const &pixelsToFieldAngle, | ||
| lsst::geom::Angle const &orientation, bool flipX, | ||
| lsst::geom::SpherePoint const &crval, | ||
| std::string const &projection = "TAN"); |
There was a problem hiding this comment.
I'm a bit concerned that the name of this function and the types of its arguments really give no information about what it's for, or more importantly, how it differs from other makeSkyWcs overloads - and while the documentation of course does that, I think the method name should be a bit more descriptive. I realize this is going against precedent (for this class, at least), but I'd prefer a name like SkyWcs::fromCameraGeom. If you'd rather a free function to go along with what exists, perhaps makeSkyWcsFromCameraGeom?
That said, really excellent clarity in the documentation for what this does. Thanks for taking the time to do that carefully.
There was a problem hiding this comment.
I plan to rename the crval argument to boresight since it is the sky position at field angle (0, 0). The current name seems more appropriate for a WCS whose reference sky position is on the detector.
Regarding the function name...I agree it's a bit vague but I have not thought of anything I like better. makeSkyWcsFromCameraGeom seems too long, though it is tolerable as long as we violate our standards and abbreviate "geometry".
On your final review comments you suggested we should have another signature once Detector has a reference to camera geometry. I'm not entirely sure it's needed, but it could be handy (especially for handling ACTUAL_PIXELS if we ever do use those, as otherwise one will have to pass in two transforms -- actual pixels to pixels and pixels to field angle). Perhaps that one can be called makeSkyWcsFromDetector and this one can stay as it is. I think makeSkyWcsFromDetector would have to go in cameraGeom because cameraGeom depends on geom.
In any case, if we have cameraGeom::makeSkyWcsFromDetector then I think makeSkyWcs is probably a better choice than makeSkyWcsFromCameraGeom in order to reduce the number of specialized functions that do almost the same thing but have different names.
include/lsst/afw/geom/SkyWcs.h
Outdated
| * @param[in] orientation Position angle of focal plane +Y, measured from N through E at crval. | ||
| * At 0 degrees, +Y is along N and +X is along W/E if flipX false/true. | ||
| * At 90 degrees, +Y is along E and +X is along N/S if flipX false/true. | ||
| * @param[in] flipX Fip x axis? See orientation for details. |
include/lsst/afw/geom/SkyWcs.h
Outdated
| * Construct a FITS SkyWcs from camera geometry | ||
| * | ||
| * @param[in] pixelsToFieldAngle Transformation from \ref afwCameraGeomPIXELS "pixels" | ||
| * to \ref afwCameraGeomFIELD_ANGLE "field angle" (in radians). |
Add a new overload of `makeSkyWcs` that accepts a PIXELS to FIELD_ANGLE transform, orientation, flipX and crval. Include two tests: one with no distortion and one with distortion.
No description provided.