Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-12447

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

    XMLWordPrintable

    Details

    • Story Points:
      5
    • Epic Link:
    • Sprint:
      AP S18-2, AP S18-3
    • Team:
      Alert Production

      Description

      Implement RFC-392: make Detector.transform and Camera.transform support lists of points (or single points) and delete the CameraPoint class. Removing the CameraPoint class also requires a minor change to Camera.findDetectors and findDetectorsList.

        Attachments

          Issue Links

            Activity

            No builds found.
            rowen Russell Owen created issue -
            rowen Russell Owen made changes -
            Field Original Value New Value
            Link This issue is triggered by RFC-392 [ RFC-392 ]
            rowen Russell Owen made changes -
            Sprint AP S18-2 [ 677 ]
            rowen Russell Owen made changes -
            Rank Ranked higher
            swinbank John Swinbank made changes -
            Epic Link DM-9679 [ 30784 ]
            swinbank John Swinbank made changes -
            Sprint AP S18-2 [ 677 ] AP S18-2, AP S18-3 [ 677, 683 ]
            rowen Russell Owen made changes -
            Status To Do [ 10001 ] In Progress [ 3 ]
            rowen Russell Owen made changes -
            Description Implement RFC-392: remove TransformMap.transform, Camera.transform and Detector.transform and the CameraPoint class. Modify existing code to use a {{TransformPoint2ToPoint2}} obtained from a {{Camera}} or {{AmpInfo}} object. Implement RFC-392: remove TransformMap.transform, Camera.transform and Detector.transform and delete the CameraPoint class. Modify existing code to use a {{TransformPoint2ToPoint2}} obtained from {{Camera.getTransform}} or {{Detector.getTransform}}.

            This also requires a few changes to Camera.findDetectors (change the single camera point argument to a point and a camera sys) and findDetectorsList (the first argument will be a list of points, not camera points).
            rowen Russell Owen made changes -
            Component/s ip_isr [ 10730 ]
            rowen Russell Owen made changes -
            Component/s jointcal [ 13411 ]
            Component/s meas_mosaic [ 10742 ]
            Component/s obs_sdss [ 10763 ]
            rowen Russell Owen made changes -
            Reviewers Paul Price [ price ]
            Status In Progress [ 3 ] In Review [ 10004 ]
            Hide
            price Paul Price added a comment - - edited

            Everywhere there's a getTransform(fromSys, toSys) method, there needs to be an applyTransform(points, fromSys, toSys) method (or similar name) that does getTransform(fromSys, toSys).applyForward(points) because most of the times you call getTransform you want to applyForward immediately after.

            Show
            price Paul Price added a comment - - edited Everywhere there's a getTransform(fromSys, toSys) method, there needs to be an applyTransform(points, fromSys, toSys) method (or similar name) that does getTransform(fromSys, toSys).applyForward(points) because most of the times you call getTransform you want to applyForward immediately after.
            rowen Russell Owen made changes -
            Component/s meas_base [ 10750 ]
            Hide
            price Paul Price added a comment -

            Would you please check that the use in obs_subaru is still OK?

            Show
            price Paul Price added a comment - Would you please check that the use in obs_subaru is still OK?
            Hide
            price Paul Price added a comment -

            Comments on the afw GitHub PR.

            Show
            price Paul Price added a comment - Comments on the afw GitHub PR.
            price Paul Price made changes -
            Status In Review [ 10004 ] Reviewed [ 10101 ]
            Hide
            rowen Russell Owen added a comment -

            I agree with Paul Price that losing the transform method is overkill, despite the RFC. I still think CameraPoint is unnecessary and mostly gets in the way. I re-added transform using the API transform(points, fromSys, toSys) where points can be a single Point2D or a vector of Point2D. This is as a separate commit where relevant, since that helps separate getting rid of the old transform method from adding the new one.

            I checked the code Paul Price asked about in obs_subaru and found that it would work but was a bit inefficient, so I tweaked it. Unfortunately it has no unit test, so that was a "best effort" change.

            Please have another look.

            One question is whether I should simplify the code for Detector by using templates for the CameraSys and CameraSysPrefix overloads of the various methods. I did this for Detector.transform in order to avoid a lot of code duplication (since it takes two coordinate system arguments). It is less necessary for the other methods, but it would reduce duplication. However, that will require the code to always call makeCameraSys on the system argument. I am pretty sure the compiler will optimize that call away for the CameraSys version because makeCameraSys is an inline function. It is not an expensive call in any case. Jim Bosch or Krzysztof Findeisen may wish to weigh in on this.

            Show
            rowen Russell Owen added a comment - I agree with Paul Price that losing the transform method is overkill, despite the RFC. I still think CameraPoint is unnecessary and mostly gets in the way. I re-added transform using the API transform(points, fromSys, toSys) where points can be a single Point2D or a vector of Point2D . This is as a separate commit where relevant, since that helps separate getting rid of the old transform method from adding the new one. I checked the code Paul Price asked about in obs_subaru and found that it would work but was a bit inefficient, so I tweaked it. Unfortunately it has no unit test, so that was a "best effort" change. Please have another look. One question is whether I should simplify the code for Detector by using templates for the CameraSys and CameraSysPrefix overloads of the various methods. I did this for Detector.transform in order to avoid a lot of code duplication (since it takes two coordinate system arguments). It is less necessary for the other methods, but it would reduce duplication. However, that will require the code to always call makeCameraSys on the system argument. I am pretty sure the compiler will optimize that call away for the CameraSys version because makeCameraSys is an inline function. It is not an expensive call in any case. Jim Bosch or Krzysztof Findeisen may wish to weigh in on this.
            rowen Russell Owen made changes -
            Status Reviewed [ 10101 ] In Review [ 10004 ]
            rowen Russell Owen made changes -
            Description Implement RFC-392: remove TransformMap.transform, Camera.transform and Detector.transform and delete the CameraPoint class. Modify existing code to use a {{TransformPoint2ToPoint2}} obtained from {{Camera.getTransform}} or {{Detector.getTransform}}.

            This also requires a few changes to Camera.findDetectors (change the single camera point argument to a point and a camera sys) and findDetectorsList (the first argument will be a list of points, not camera points).
            Implement RFC-392: make Detector.transform and Camera.transform support lists of points (or single points) and delete the CameraPoint class. Removing the CameraPoint class also requires a minor change to Camera.findDetectors and findDetectorsList.
            Summary Remove TransformMap.transform, Camera.transform and Detector.transform Make Detector.transform and Camera.transform support lists of points
            rowen Russell Owen made changes -
            Component/s pipe_tasks [ 10726 ]
            Hide
            krzys Krzysztof Findeisen added a comment -

            I think that sounds reasonable, assuming we still need to keep CameraSysPrefix (I assume Detector is still a class, not a template?).

            Show
            krzys Krzysztof Findeisen added a comment - I think that sounds reasonable, assuming we still need to keep CameraSysPrefix (I assume Detector is still a class, not a template?).
            Hide
            rowen Russell Owen added a comment -

            I think it is important to retain CameraSysPrefix; it allows us to offer a standard way of expressing "pixels" without specifying the detector, e.g. posFocalPlane = detector.transform(posPixels, PIXELS, FOCAL_PLANE).

            Detector is still a class. The only thing gone is CameraPoint, which combines a position and a CameraSys.

            Show
            rowen Russell Owen added a comment - I think it is important to retain CameraSysPrefix ; it allows us to offer a standard way of expressing "pixels" without specifying the detector, e.g. posFocalPlane = detector.transform(posPixels, PIXELS, FOCAL_PLANE) . Detector is still a class. The only thing gone is CameraPoint , which combines a position and a CameraSys .
            Hide
            price Paul Price added a comment -

            Looks good.

            Show
            price Paul Price added a comment - Looks good.
            price Paul Price made changes -
            Status In Review [ 10004 ] Reviewed [ 10101 ]
            rowen Russell Owen made changes -
            Resolution Done [ 10000 ]
            Status Reviewed [ 10101 ] Done [ 10002 ]

              People

              Assignee:
              rowen Russell Owen
              Reporter:
              rowen Russell Owen
              Reviewers:
              Paul Price
              Watchers:
              Krzysztof Findeisen, Paul Price, Russell Owen
              Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  Jenkins Builds

                  No builds found.