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

Update cbp for changes in how Cameras and Detectors are built

    XMLWordPrintable

Details

    • Improvement
    • Status: Done
    • Resolution: Done
    • None
    • cbp
    • None

    Description

      Please update the cbp package for change in how Detectors and Cameras are built.

      I suggest you use the tickets/DM-20488 branch or base your branch off of that one. Or, if you prefer, cherry-pick those changes.

      I think the main class that needs updating is SampleCoordinateConverter in testUtils.py.
      I had a try at this myself (see attached file), and was able to make a list of Detectors and get rid of the no-longer-needed AmpInfoTable (the test code doesn't care about amplifiers), but could not figure out how to create a Camera (nor verify that the Detectors I made were correct).

      Attachments

        Issue Links

          Activity

            swinbank John Swinbank added a comment - - edited

            rowen — could you please take a look at https://github.com/lsst/cbp/pull/7 and see what you think?

            A couple of things are worth noting:

            First, some of the code in both the tests and in computeHolePositions assumes that list(cameraGeom.getNameIter()) returns a list sorted in a particular order. That doesn't seem to be true in practice (and I don't think that's a bug). I've worked around this by the expedient method of using sorted() where necessary and switching to set (rather than list) comparisons, which works (as far as the tests are concerned), but feels a little clunky. You undoubtedly have more insight into what the code should be doing than I do, so you may wish to suggest a better approach.

            Secondly, even with these changes, I see a test failure:

                    for telRotObserved in (0*degrees, -32*degrees, 167*degrees):
                        self.cco.rotAzAltObserved = telRotObserved
                        self.assertAnglesAlmostEqual(self.cco.rotAzAltObserved, telRotObserved)
                        predictedRotInternal = telRotObserved/self.cco.config.telRotScale - self.cco.config.telRotOffset
            >           self.assertAnglesAlmostEqual(self.cco.telRotInternal, predictedRotInternal)
             
            tests/test_coordinateConverter.py:466:
            _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
            /opt/lsst/software/stack/stack/miniconda3-4.7.12-984c9f7/Linux64/geom/19.0.0-2-gde8e5e3+2/python/lsst/geom/testUtils.py:84: in assertAnglesAlmostEqual
                testCase.fail("ang0 is NaN")
            E   AssertionError: ang0 is NaN
            

            After a few minutes looking at the code, I can't see how this is supposed to work (I don't see why self.cco.telRotInternal would be anything other than NaN). Further, I note that this test fails with the oldest stacks I have available, dating from far before the cameraGeom changes late last year.

            Perhaps you could take a look at this test and see if you still think it is useful?

            Oh, and I should say thanks to cmorrison for the help (and moral support!) while fighting with cameraGeom weirdness!

            swinbank John Swinbank added a comment - - edited rowen — could you please take a look at https://github.com/lsst/cbp/pull/7 and see what you think? A couple of things are worth noting: First, some of the code in both the tests and in computeHolePositions assumes that list(cameraGeom.getNameIter()) returns a list sorted in a particular order. That doesn't seem to be true in practice (and I don't think that's a bug). I've worked around this by the expedient method of using sorted() where necessary and switching to set (rather than list) comparisons, which works (as far as the tests are concerned), but feels a little clunky. You undoubtedly have more insight into what the code should be doing than I do, so you may wish to suggest a better approach. Secondly, even with these changes, I see a test failure: for telRotObserved in (0*degrees, -32*degrees, 167*degrees): self.cco.rotAzAltObserved = telRotObserved self.assertAnglesAlmostEqual(self.cco.rotAzAltObserved, telRotObserved) predictedRotInternal = telRotObserved/self.cco.config.telRotScale - self.cco.config.telRotOffset > self.assertAnglesAlmostEqual(self.cco.telRotInternal, predictedRotInternal)   tests/test_coordinateConverter.py:466: _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ /opt/lsst/software/stack/stack/miniconda3-4.7.12-984c9f7/Linux64/geom/19.0.0-2-gde8e5e3+2/python/lsst/geom/testUtils.py:84: in assertAnglesAlmostEqual testCase.fail("ang0 is NaN") E AssertionError: ang0 is NaN After a few minutes looking at the code, I can't see how this is supposed to work (I don't see why self.cco.telRotInternal would be anything other than NaN). Further, I note that this test fails with the oldest stacks I have available, dating from far before the cameraGeom changes late last year. Perhaps you could take a look at this test and see if you still think it is useful? Oh, and I should say thanks to cmorrison for the help (and moral support!) while fighting with cameraGeom weirdness!
            rowen Russell Owen added a comment - - edited

            Thank you very much for the update!

            Regarding detector order: the old camera geometry was an ordered collection of detectors, which reliably had the order in which they were defined. Apparently that is not so for the new system? In any case, your fix works. Thank you.

            Regarding that failing unit test: you are right. The test is mis-written. I intended to set the rotAzAltObserved property but instead set a non-existent attribute. I have no idea how that ever worked.

            I made a pull request https://github.com/lsst/cbp/pull/8 on the existing pull request that has a fix for that test, plus some documentation updates I would like you to consider.

            rowen Russell Owen added a comment - - edited Thank you very much for the update! Regarding detector order: the old camera geometry was an ordered collection of detectors, which reliably had the order in which they were defined. Apparently that is not so for the new system? In any case, your fix works. Thank you. Regarding that failing unit test: you are right. The test is mis-written. I intended to set the rotAzAltObserved property but instead set a non-existent attribute. I have no idea how that ever worked. I made a pull request https://github.com/lsst/cbp/pull/8 on the existing pull request that has a fix for that test, plus some documentation updates I would like you to consider.

            Thanks Russell!

            Regarding detector order: the old camera geometry was an ordered collection of detectors, which reliably had the order in which they were defined. Apparently that is not so for the new system?

            I'm actually a bit surprised that it no longer seems to be the case, but since it isn't documented as part of the interface, and since it's easy enough to work around, I'm not going to worry about it too hard!

            I have accepted your PR (thank you!) and merged to master. Marking this as done.

            swinbank John Swinbank added a comment - Thanks Russell! Regarding detector order: the old camera geometry was an ordered collection of detectors, which reliably had the order in which they were defined. Apparently that is not so for the new system? I'm actually a bit surprised that it no longer seems to be the case, but since it isn't documented as part of the interface, and since it's easy enough to work around, I'm not going to worry about it too hard! I have accepted your PR (thank you!) and merged to master . Marking this as done.

            People

              swinbank John Swinbank
              rowen Russell Owen
              Russell Owen
              John Swinbank, Russell Owen
              Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved:

                Jenkins

                  No builds found.