Details
-
Type:
Improvement
-
Status: Done
-
Resolution: Done
-
Fix Version/s: None
-
Component/s: cbp
-
Labels:None
-
Story Points:2
-
Epic Link:
-
Team:Alert Production
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
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.
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 = telRotObservedself.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 assertAnglesAlmostEqualtestCase.fail("ang0 is NaN")E AssertionError: ang0 is NaNAfter 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!