# Update cbp for changes in how Cameras and Detectors are built

XMLWordPrintable

#### Details

• Type: Improvement
• Status: Done
• Resolution: Done
• Fix Version/s: None
• Component/s:
• Labels:
None
• Story Points:
2
• Team:

#### 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

1. testUtils.py
11 kB

#### Activity

Hide
John Swinbank added a comment - - 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!

Show
John Swinbank added a comment - - 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!
Hide
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.

Show
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.
Hide
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.

Show
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

Assignee:
John Swinbank
Reporter:
Russell Owen
Reviewers:
Russell Owen
Watchers:
John Swinbank, Russell Owen