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

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

    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

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

              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:

                  Summary Panel