John Swinbank added a comment - 07/Mar/20 12:57 AM - edited Russell Owen — 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 Chris Morrison [X] for the help (and moral support!) while fighting with cameraGeom weirdness!

Russell Owen added a comment - 09/Mar/20 11:19 PM - 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.

John Swinbank added a comment - 09/Mar/20 11:46 PM 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.