Uploaded image for project: 'Request For Comments'
  1. Request For Comments
  2. RFC-392

Make Camera.transform and Detector.transform support lists of points and remove CameraPoint

    XMLWordPrintable

    Details

    • Type: RFC
    • Status: Implemented
    • Resolution: Done
    • Component/s: DM
    • Labels:
      None

      Description

      It turns out that TransformMap.transform is not very efficient because each time it is called a transform must be copied (after a lookup, which is efficient). This inefficiency extends to Camera.transform and Detector.transform because they both call TransformMap.transform.

      In the interest of simplicity and providing one preferred way to do things, I propose that we eliminate these methods and instead make the user obtain a transform via getTransform and convert points in the usual way, with applyForward. Note that this will also eliminate the CameraPoint class, since it is only used with the transform methods in question.

      I personally feel that use of CameraSys to look up transforms, coupled with reasonable names for the resulting transforms provides sufficient clarity. I do not feel the use of CameraPoint adds significant additional clarity and it is awkward because it only works for single points, whereas transforms are most efficient when transforming lists of points.

        Attachments

          Issue Links

            Activity

            Hide
            tjenness Tim Jenness added a comment -

            Russell Owen what's the status of this RFC?

            Show
            tjenness Tim Jenness added a comment - Russell Owen what's the status of this RFC?
            Hide
            rowen Russell Owen added a comment - - edited

            That's a good question. There was initial discussion but no responses to my answers. Robert Lupton told me he wants more time to think about this so I'll extend the date another week.

            Show
            rowen Russell Owen added a comment - - edited That's a good question. There was initial discussion but no responses to my answers. Robert Lupton told me he wants more time to think about this so I'll extend the date another week.
            Hide
            jbosch Jim Bosch added a comment -

            I have no problem with this in the sense that I think the Transform objects are a much more important API than the higher-level transformation functions that are proposed to be removed (in the sense that lower level code will often operate on an opaque Transform object).

            I do think we are losing something in dropping the ability to do any type- or unit-safety on camera positions, but I think that type safety is really hard to implement rigorously in a system with general transform compositions, so much so that I think it's probably not worht the effort. A halfhearted attempt to include type- or unit-safety in just a few interfaces (which is what the proposed-to-be-removed functions do) doesn't strike me as nearly as valuable, and even though it's easier to implement, I'm not sure it's worth keeping either.

            Overall, I am essentially ambivalent about this change.

            Show
            jbosch Jim Bosch added a comment - I have no problem with this in the sense that I think the Transform objects are a much more important API than the higher-level transformation functions that are proposed to be removed (in the sense that lower level code will often operate on an opaque Transform object). I do think we are losing something in dropping the ability to do any type- or unit-safety on camera positions, but I think that type safety is really hard to implement rigorously in a system with general transform compositions, so much so that I think it's probably not worht the effort. A halfhearted attempt to include type- or unit-safety in just a few interfaces (which is what the proposed-to-be-removed functions do) doesn't strike me as nearly as valuable, and even though it's easier to implement, I'm not sure it's worth keeping either. Overall, I am essentially ambivalent about this change.
            Hide
            rowen Russell Owen added a comment -

            Adopted as stated in the Description

            Show
            rowen Russell Owen added a comment - Adopted as stated in the Description
            Hide
            rowen Russell Owen added a comment -

            Having implemented this on DM-12447 Paul Price (the reviewer) and I realized it was a bit clumsy. What is really wanted is a way to transform a list of points all at once, since calling Camera.transform or Detector.transform in a loop is inefficient. So what I have done instead is:

            • Changed Camera.transform and Detector.transform to support lists of points and single points (Point2D in both cases) to encourage users to convert all points at once. The argument list has changed from (cameraPoint, toSys) to (pointOrPoints, fromSys, toSys). Detector supports camera system prefixes such as PIXELS and Camera does not.
            • Added Camera.getTransform(fromSys, toSys) and Detector.getTransform(fromSys, toSys). Both have proven to be useful some cases. Again, Detector supports camera system prefixes such as PIXELS and Camera does not.
            • Eliminated CameraPoint as stated above. It seems to get in the way rather than be helpful.

            In my opinion it simplifies existing code, but I encourage you to look at the pull requests for DM-12447 and decide for yourself. If this is controversial I am happy to re-open this RFC.

            Show
            rowen Russell Owen added a comment - Having implemented this on DM-12447 Paul Price (the reviewer) and I realized it was a bit clumsy. What is really wanted is a way to transform a list of points all at once, since calling Camera.transform or Detector.transform in a loop is inefficient. So what I have done instead is: Changed Camera.transform and Detector.transform to support lists of points and single points ( Point2D in both cases) to encourage users to convert all points at once. The argument list has changed from (cameraPoint, toSys) to (pointOrPoints, fromSys, toSys) . Detector supports camera system prefixes such as PIXELS and Camera does not. Added Camera.getTransform(fromSys, toSys) and Detector.getTransform(fromSys, toSys) . Both have proven to be useful some cases. Again, Detector supports camera system prefixes such as PIXELS and Camera does not. Eliminated CameraPoint as stated above. It seems to get in the way rather than be helpful. In my opinion it simplifies existing code, but I encourage you to look at the pull requests for DM-12447 and decide for yourself. If this is controversial I am happy to re-open this RFC.

              People

              Assignee:
              rowen Russell Owen
              Reporter:
              rowen Russell Owen
              Watchers:
              Jim Bosch, John Parejko, Krzysztof Findeisen, Robert Lupton, Russell Owen, Scott Daniel, Tim Jenness
              Votes:
              0 Vote for this issue
              Watchers:
              7 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:
                Planned End:

                  CI Builds

                  No builds found.