DM-12447: Remove TransformMap.transform, Camera.transform and Detector.transform #321

r-owen · 2018-02-26T17:08:45Z

and CameraPoint

PaulPrice · 2018-02-26T18:59:40Z

Can you state the reason for the removal in the commit log?

PaulPrice

I think we absolutely need a method that will do getTransform followed by applyForward.

PaulPrice · 2018-02-26T19:05:54Z

examples/cameraGeomExample.py

-detPixelsSys = det.makeCameraSys(cameraGeom.PIXELS)
-detPointOnSpecifiedDetector = camera.transform(fieldPoint, detPixelsSys)
-detPointOnFoundDetector = camera.transform(fieldPoint, cameraGeom.PIXELS)
-assert detPointOnFoundDetector.getCameraSys() == detPixelsSys  # same detector


Why is this gone? Does this represent loss of functionality?

Having Camera.transform search for a detector is gone. I don't think it's practical to implement it with CameraPoint gone. In my opinion it was never a good idea, as it could be quite slow, which could surprise users. I would much rather have users explicitly search for a detector if necessary.

Note that I found no instances where this feature was used, other than this example.

PaulPrice · 2018-02-26T19:13:13Z

include/lsst/afw/cameraGeom/CameraPoint.h

-/**
- * A Point2D with associated camera coordinate system
- */
-class CameraPoint {


This used to be viewed as an important element of the design that made sure the user knew which coordinate system the points are in. Please explain in the commit log message why it is now being jettisoned.

PaulPrice · 2018-02-26T19:24:06Z

python/lsst/afw/cameraGeom/camera.py

-        """!Transform a CameraPoint with a CameraSys to another CameraSys.
+            return det.getTransformMap().getTransform(fromSys, toSys)
+        else:
+            return self.getTransformMap().getTransform(fromSys, toSys)


I think the logic above should be refactored to eliminate duplicated code:

det = self if fromSys.hasDetectorName(): det = self[fromSys.getDetectorName()] elif toSys.hasDetectorName(): det = self[toSys.getDetectorName()] return det.getTransformMap().getTransform(fromSys, toSys)

Your suggested new code does not do the same thing because the else clause handles the camera's own transform map, not a detector's transform map.

I suppose we could retrieve the transform map. It will be a bit awkward since we then have to assume the code will never leave the transform map equal to None (which will be true initially, but perhaps not if the code evolves) or check for None even though it should be impossible. That is why I chose to use the current technique.

PaulPrice · 2018-02-26T19:41:55Z

python/lsst/afw/cameraGeom/camera.py

-            cameraPoint, self._nativeCameraSys)
+        # convert `point` to the native coordinate system
+        transform = self._getTransformFromOneTransformMap(cameraSys, self._nativeCameraSys)
+        nativePoint = transform.applyForward(point)

        detectorList = []


Why is there a list of detectors, as if a point could be on multiple detectors?

Because a point can be on multiple detectors. I don't believe any instruments presently use that capability, but an imaging instrument with a beamsplitter certainly would.

Note that this is a pre-existing capability that has been retained.

PaulPrice · 2018-02-26T19:47:01Z

python/lsst/afw/cameraGeom/camera.py

@@ -196,31 +156,19 @@ def getTransformMap(self):
        """
        return self._transformMap

-    def transform(self, cameraPoint, toSys):


Don't want a camera.transform(point, fromSys, toSys)?

I added that in a subsequent commit.

RobertLuptonTheGood · 2018-02-27T19:05:10Z

What is the functionality that you've removed (the one that you didn't find in the live)?

r-owen · 2018-02-27T19:06:27Z

@RobertLuptonTheGood Having Camera.transform search for a detector (if a camera system prefix was specified). It is a very expensive operation and it is doubtful that anyone reading the code would realize that.

PaulPrice · 2018-02-27T18:41:55Z

include/lsst/afw/cameraGeom/Detector.h

+     *
+     * @tparam FromSysT  Class of fromSys: one of CameraSys or CameraSysPrefix
+     * @tparam ToSysT  Class of toSys: one of CameraSys or CameraSysPrefix
+     * @param[in] point  Camera point to transform


points, plural.

PaulPrice · 2018-02-27T18:42:12Z

include/lsst/afw/cameraGeom/Detector.h

+     * @tparam FromSysT  Class of fromSys: one of CameraSys or CameraSysPrefix
+     * @tparam ToSysT  Class of toSys: one of CameraSys or CameraSysPrefix
+     * @param[in] point  Camera point to transform
+     * @param[in] fromSys  Camera coordinate system of `point`


PaulPrice · 2018-02-27T18:43:14Z

python/lsst/afw/cameraGeom/camera.py

@@ -173,3 +165,25 @@ def getTransform(self, fromSys, toSys):
        fromSysToNative = self._getTransformFromOneTransformMap(fromSys, self._nativeCameraSys)
        nativeToToSys = self._getTransformFromOneTransformMap(self._nativeCameraSys, toSys)
        return fromSysToNative.then(nativeToToSys)
+
+    def getTransformMap(self):


Why a getter instead of the more pythonic property?

That is existing functionality (I think github is a bit confused about what changed).

It's a good idea, but I'd rather that be a separate RFC and ticket.

PaulPrice · 2018-02-27T18:50:36Z

python/lsst/afw/cameraGeom/detector.cc

@@ -70,7 +102,7 @@ PYBIND11_PLUGIN(_detector) {
                     table::AmpInfoCatalog const &, Orientation const &, geom::Extent2D const &,
                     TransformMap::Transforms const &, Detector::CrosstalkMatrix const &>(),
            "name"_a, "id"_a, "type"_a, "serial"_a, "bbox"_a, "ampInfoCatalog"_a, "orientation"_a,
-            "pixelSize"_a, "transforms"_a, "crosstalk"_a=Detector::CrosstalkMatrix());
+            "pixelSize"_a, "transforms"_a, "crosstalk"_a = Detector::CrosstalkMatrix());


Formatting change shouldn't be included.

PaulPrice · 2018-02-27T18:59:37Z

python/lsst/afw/cameraGeom/utils.py

-        posFocalPlane = fieldAngleToFocalPlane.applyForward(posFieldAngle)
+
+    # compute focal plane positions corresponding to field angles field_gridx, field_gridy
+    posFieldAngleRadiansList = [afwGeom.Point2D(x.asRadians(), y.asRadians())


I think you're turning Hungarian... Having such long variable names isn't good.

I am Hungarian. But I'll get rid of "Radians"

PaulPrice · 2018-02-27T19:06:48Z

python/lsst/afw/cameraGeom/utils.py

+    # compute focal plane positions corresponding to field angles field_gridx, field_gridy
+    posFieldAngleRadiansList = [afwGeom.Point2D(x.asRadians(), y.asRadians())
+                                for x, y in zip(field_gridx, field_gridy)]
+    posFocalPlaneList = camera.transform(posFieldAngleRadiansList, FIELD_ANGLE, FOCAL_PLANE)


Why isn't std::pair<Angle, Angle> not a supported endpoint type?

I think that adds too much complexity, especially because it also requires adding support for std::vector<std::pair<Angle, Angle>> and adding a new Endpoint type. In any case it is clearly out of scope for this ticket.

PaulPrice · 2018-02-27T19:09:22Z

src/cameraGeom/Detector.cc

+std::vector<geom::Point2D> Detector::transform(std::vector<geom::Point2D> const &points,
+                                               FromSysT const &fromSys, ToSysT const &toSys) const {
+    return _transformMap.transform(points, makeCameraSys(fromSys), makeCameraSys(toSys));
+}


I'm surprised these aren't defined in the header, since they're so simple.

I prefer to define functions in the .cc file unless there's a good reason to do otherwse (e.g. inlining a method. It reduces clutter in the header saves on compilation time if something changes.

PaulPrice · 2018-02-27T19:10:18Z

src/cameraGeom/Detector.cc

+    template std::shared_ptr<geom::TransformPoint2ToPoint2> Detector::getTransform(FROMSYS const &,          \
+                                                                                   TOSYS const &) const;     \
+    template geom::Point2D Detector::transform(geom::Point2D const &, FROMSYS const &, TOSYS const &) const; \
+    template std::vector<geom::Point2D> Detector::transform(std::vector<geom::Point2D> const &,              \


For the same reason we don't line up equal signs in a list of equations, I suggest not spacing out the backslashes.

clang-format

PaulPrice · 2018-02-27T19:12:13Z

tests/test_detector.py

+            pixPoint = dw.detector.transform(fpPoint, cameraGeom.FOCAL_PLANE, cameraGeom.PIXELS)
+            for i in range(2):
+                self.assertAlmostEqual(
+                    fpPoint[i]/dw.pixelSize[i] + pixOffset[i], pixPoint[i])


Instead of iteration, can't you do this with Point and Extent arithmetic?

Interesting idea. I tried fpPoint/dw.pixelSize + pixOffset but it was rejected with TypeError: unsupported operand type(s) for /: 'lsst.afw.geom.coordinates.coordinates.Point2D' and 'lsst.afw.geom.coordinates.coordinates.Extent2D'

I'll stick to this for now -- it is existing code and known to work.

Remove `CameraPoint` Add support for transforming lists of points. Remove support for `Camera.transform` searching for a suitable detector. It was very inefficient and it would be difficult to tell that code was using it. Fortunately it was not used anywhere.

Also fix broken links.

r-owen · 2018-02-27T22:20:01Z

I rebased my changes

r-owen force-pushed the tickets/DM-12447 branch from b9bad62 to 864d052 Compare February 26, 2018 18:53

PaulPrice approved these changes Feb 26, 2018

View reviewed changes

r-owen force-pushed the tickets/DM-12447 branch from fa58ac7 to df0a8ae Compare February 27, 2018 02:16

PaulPrice approved these changes Feb 27, 2018

View reviewed changes

r-owen force-pushed the tickets/DM-12447 branch from df0a8ae to 2802a1a Compare February 27, 2018 22:07

r-owen added 4 commits February 27, 2018 14:12

Clang-format detector.cc

055735b

Fix doxygen warning in camera.py

6b2845b

Update cameraGeom.dox for new API

607a0d2

Also fix broken links.

r-owen force-pushed the tickets/DM-12447 branch from 2802a1a to 607a0d2 Compare February 27, 2018 22:19

r-owen merged commit 607a0d2 into master Feb 27, 2018

ktlim deleted the tickets/DM-12447 branch August 25, 2018 06:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-12447: Remove TransformMap.transform, Camera.transform and Detector.transform #321

DM-12447: Remove TransformMap.transform, Camera.transform and Detector.transform #321

r-owen commented Feb 26, 2018

PaulPrice commented Feb 26, 2018

PaulPrice left a comment

PaulPrice Feb 26, 2018

r-owen Feb 27, 2018

PaulPrice Feb 26, 2018

PaulPrice Feb 26, 2018

r-owen Feb 27, 2018

PaulPrice Feb 26, 2018

r-owen Feb 27, 2018 •

edited

PaulPrice Feb 26, 2018

r-owen Feb 27, 2018

RobertLuptonTheGood commented Feb 27, 2018 via email

r-owen commented Feb 27, 2018 •

edited

PaulPrice Feb 27, 2018

PaulPrice Feb 27, 2018

PaulPrice Feb 27, 2018

r-owen Feb 27, 2018

PaulPrice Feb 27, 2018

PaulPrice Feb 27, 2018

r-owen Feb 27, 2018 •

edited

PaulPrice Feb 27, 2018

r-owen Feb 27, 2018

PaulPrice Feb 27, 2018

r-owen Feb 27, 2018

PaulPrice Feb 27, 2018

r-owen Feb 27, 2018

PaulPrice Feb 27, 2018

r-owen Feb 27, 2018

r-owen commented Feb 27, 2018

DM-12447: Remove TransformMap.transform, Camera.transform and Detector.transform #321

DM-12447: Remove TransformMap.transform, Camera.transform and Detector.transform #321

Conversation

r-owen commented Feb 26, 2018

PaulPrice commented Feb 26, 2018

PaulPrice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r-owen Feb 27, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RobertLuptonTheGood commented Feb 27, 2018 via email

r-owen commented Feb 27, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r-owen Feb 27, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r-owen commented Feb 27, 2018

r-owen Feb 27, 2018 •

edited

r-owen commented Feb 27, 2018 •

edited

r-owen Feb 27, 2018 •

edited