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

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

    XMLWordPrintable

Details

    • 5
    • AP S18-2, AP S18-3
    • 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 ]
            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.

            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 ]
            price Paul Price added a comment -

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

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

            Comments on the afw GitHub PR.

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

            I agree with 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 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. jbosch or krzys may wish to weigh in on this.

            rowen Russell Owen added a comment - I agree with 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 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. jbosch or krzys 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 ]

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

            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?).

            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.

            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 .
            price Paul Price added a comment -

            Looks good.

            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

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

              Dates

                Created:
                Updated:
                Resolved:

                Jenkins

                  No builds found.