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

obs_test needs to override map_camera and std_camera

    XMLWordPrintable

    Details

    • Type: Bug
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: obs_test
    • Labels:
      None

      Description

      The Butler can't get a camera unless the map_camera and std_camera are defined correctly. In most cases the camera can be built by the map_camera method. In the case of obs_test, the camera is built in the constructor of the Mapper, so std_camera should just return the camera attribute.

        Attachments

          Issue Links

            Activity

            Hide
            rowen Russell Owen added a comment -

            Kian-Tat Lim Yes TestMapper would override _makeCamera() to construct and return the camera. I am not sure under what circumstances self.camera can reasonably be None, but if there are such circumstances then _makeCamera() could be overridden to return None.

            Show
            rowen Russell Owen added a comment - Kian-Tat Lim Yes TestMapper would override _makeCamera() to construct and return the camera. I am not sure under what circumstances self.camera can reasonably be None, but if there are such circumstances then _makeCamera() could be overridden to return None.
            Hide
            rowen Russell Owen added a comment -

            Kian-Tat Lim I pushed the change I mentioned to see how well it worked. What I found: I needed to add a small camera to daf_butlerUtils in order for the unit tests that made a CameraMapper to be happy. I am running the Jenkins CI system to see if any other packages will have the same issue. If it breaks many packages then it may not be worth the headache. On the other hand, if it just broke 3 unit tests in daf_butlerUtils, all of which were fixed by actually constructing a camera, then I think it may be worthwhile because it eliminates a source of silent errors.

            The changes in obs_test were trivial; I replaced a single line of code in the constructor for TestMapper with a method that contains a single line of code. So it didn't reduce lines of code, but did make it a bit clearer what was going on. It would be a much nicer win for an obs_* package that wanted to construct a complex camera in some way other than using a paf file.

            Show
            rowen Russell Owen added a comment - Kian-Tat Lim I pushed the change I mentioned to see how well it worked. What I found: I needed to add a small camera to daf_butlerUtils in order for the unit tests that made a CameraMapper to be happy. I am running the Jenkins CI system to see if any other packages will have the same issue. If it breaks many packages then it may not be worth the headache. On the other hand, if it just broke 3 unit tests in daf_butlerUtils, all of which were fixed by actually constructing a camera, then I think it may be worthwhile because it eliminates a source of silent errors. The changes in obs_test were trivial; I replaced a single line of code in the constructor for TestMapper with a method that contains a single line of code. So it didn't reduce lines of code, but did make it a bit clearer what was going on. It would be a much nicer win for an obs_* package that wanted to construct a complex camera in some way other than using a paf file.
            Hide
            rowen Russell Owen added a comment -

            Please pronounce on whether it is acceptable to raise an exception in _makeCamera if policy does not contain "camera". The old behavior is to set the camera to None (so _makeCamera could silently do that, or issue a warning and do that). I prefer an exception because I think it is weird to silently not construct a camera, but it is a change in behavior. The code I pushed passes Jenkins.

            I could issue an RFC, but it's such a minor change that I don't think that is justified.

            It is trivial to override _makeCamera to return None if the users of the CameraMapper knows no camera is needed or wanted.

            Show
            rowen Russell Owen added a comment - Please pronounce on whether it is acceptable to raise an exception in _makeCamera if policy does not contain "camera". The old behavior is to set the camera to None (so _makeCamera could silently do that, or issue a warning and do that). I prefer an exception because I think it is weird to silently not construct a camera, but it is a change in behavior. The code I pushed passes Jenkins. I could issue an RFC, but it's such a minor change that I don't think that is justified. It is trivial to override _makeCamera to return None if the users of the CameraMapper knows no camera is needed or wanted.
            Hide
            ktl Kian-Tat Lim added a comment -

            Yes, I think it's OK to have _makeCamera rely on a camera item in the policy.

            Show
            ktl Kian-Tat Lim added a comment - Yes, I think it's OK to have _makeCamera rely on a camera item in the policy.
            Hide
            rowen Russell Owen added a comment -

            I renamed daf_butlerUtils branch u/ktlim/DM-3670 to tickets/DM-3670, ran Jenkins CI one more time, then merged daf_butlerUtils and obs_test to master and pushed.

            Show
            rowen Russell Owen added a comment - I renamed daf_butlerUtils branch u/ktlim/ DM-3670 to tickets/ DM-3670 , ran Jenkins CI one more time, then merged daf_butlerUtils and obs_test to master and pushed.

              People

              Assignee:
              rowen Russell Owen
              Reporter:
              krughoff Simon Krughoff
              Reviewers:
              Kian-Tat Lim
              Watchers:
              Kian-Tat Lim, Robert Lupton, Russell Owen, Simon Krughoff
              Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved: