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

            No builds found.
            rowen Russell Owen created issue -
            rowen Russell Owen made changes -
            Field Original Value New Value
            Link This issue relates to DM-20488 [ DM-20488 ]
            rowen Russell Owen made changes -
            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.
            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, and was able to a list of Detectors, but could not figure out how to create a Camera.
            SampleCoordinateConverter._makeAmpInfoCatalog can almost certainly be deleted, since the cbp code does not care about amplifiers (I only created an AmpInfoTable because it was required to make a Detector, and that is no longer the case.

            I have attached my attempted update of testUtils.py in case it is a useful starting point.
            rowen Russell Owen made changes -
            Attachment testUtils.py [ 41940 ]
            rowen Russell Owen made changes -
            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, and was able to a list of Detectors, but could not figure out how to create a Camera.
            SampleCoordinateConverter._makeAmpInfoCatalog can almost certainly be deleted, since the cbp code does not care about amplifiers (I only created an AmpInfoTable because it was required to make a Detector, and that is no longer the case.

            I have attached my attempted update of testUtils.py in case it is a useful starting point.
            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).
            swinbank John Swinbank made changes -
            Epic Link DM-22588 [ 427655 ]
            ktl Kian-Tat Lim made changes -
            Link This issue is triggered by RFC-658 [ RFC-658 ]
            swinbank John Swinbank made changes -
            Status To Do [ 10001 ] In Progress [ 3 ]
            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!
            swinbank John Swinbank made changes -
            Reviewers Russell Owen [ rowen ]
            Status In Progress [ 3 ] In Review [ 10004 ]
            swinbank John Swinbank made changes -
            Team DM Science [ 12218 ] Alert Production [ 10300 ]
            swinbank John Swinbank made changes -
            Epic Link DM-22588 [ 427655 ] DM-22484 [ 427311 ]
            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.
            rowen Russell Owen made changes -
            Status In Review [ 10004 ] Reviewed [ 10101 ]

            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.
            swinbank John Swinbank made changes -
            Resolution Done [ 10000 ]
            Status Reviewed [ 10101 ] Done [ 10002 ]
            swinbank John Swinbank made changes -
            Link This issue blocks DM-23830 [ DM-23830 ]
            swinbank John Swinbank made changes -
            Epic Link DM-22484 [ 427311 ] DM-24339 [ 433026 ]
            swinbank John Swinbank made changes -
            Epic Link DM-24339 [ 433026 ] DM-23740 [ 431397 ]
            swinbank John Swinbank made changes -
            Assignee Merlin Fisher-Levine [ mfisherlevine ] John Swinbank [ swinbank ]
            swinbank John Swinbank made changes -
            Epic Link DM-23740 [ 431397 ] DM-22484 [ 427311 ]

            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.