DM-11268: Detector: add crosstalk matrix #264
Conversation
PaulPrice
force-pushed
the
tickets/DM-11268
branch
from
dbcda72
to
2350101
Compare
| @@ -45,6 +46,19 @@ class DetectorConfig(pexConfig.Config): | |||
| transposeDetector = pexConfig.Field( | |||
| "Transpose the pixel grid before orienting in focal plane?", bool) | |||
|
|
|||
| crosstalk = pexConfig.ListField( | |||
| dtype=float, | |||
| doc=("Flattened crosstalk coefficient matrix; should have nAmps x nAmps entries. " | |||
There was a problem hiding this comment.
I don't like that it has to be provided in flattened form. Is that really necessary?
There was a problem hiding this comment.
Yes. We don't have a MatrixField in pex_config.
| """Return a 2-D numpy array of crosstalk coefficients of the proper shape""" | ||
| if not self.crosstalk: | ||
| return None | ||
| return np.array(self.crosstalk, dtype=np.float32).reshape((numAmps, numAmps)) |
There was a problem hiding this comment.
Should you have a try:except here for if the user requests the wrong numAmps (wrong defined as "cannot be reshaped to that square")?
| @@ -104,6 +106,9 @@ def __init__(self, | |||
| CameraSys(TAN_PIXELS, self.name): pixelToTanPixel, | |||
| CameraSys(ACTUAL_PIXELS, self.name): afwGeom.RadialXYTransform([0, 0.95, 0.01]), | |||
| } | |||
| if crosstalk is None: | |||
| crosstalk = [[0.0 for _ in range(numAmps)] for _ in range(numAmps)] | |||
There was a problem hiding this comment.
Shouldn't this be [0,0] (or maybe []) based on the logic in afw/cameraGeom/Detector.h:136? Otherwise, it looks like there is a crosstalk matrix that just happens to be all zero.
There was a problem hiding this comment.
This is a crosstalk matrix that is all zero. I could put other dummy values instead of zeros, but it wouldn't add anything to the test.
| if (hasCrosstalk()) { | ||
| auto shape = _crosstalk.getShape(); | ||
| assert(shape.size() == 2); // we've declared this as a 2D array | ||
| if (shape[0] != shape[1]) { |
There was a problem hiding this comment.
These all seem like good tests. 👍
| @@ -91,6 +93,16 @@ def addBadCameraSys(dw): | |||
| with self.assertRaises(lsst.pex.exceptions.Exception): | |||
| DetectorWrapper(modFunc=addBadCameraSys) | |||
|
|
|||
| # These break in the pybind layer | |||
| for crosstalk in ([1.0, 2.0, 3.0, 4.0, 5.0, 6.0, 7.0, 8.0, 9.0], # 1D and not numpy | |||
There was a problem hiding this comment.
You have failed crosstalk matrices, but are there tests for ok ones?
There was a problem hiding this comment.
Yes, the default in testUtils.py:110.
tests/test_detector.py
Outdated
| ): | ||
| self.assertRaises(TypeError, DetectorWrapper, crosstalk=crosstalk) | ||
| # This breaks in the Detector ctor: wrong shape | ||
| self.assertRaises(lsst.pex.exceptions.InvalidParameterError, |
There was a problem hiding this comment.
Since there are two different throws in detector.cc, you should probably have one test for each of those conditions.
|
Changes made in response to review are on the fixup commit. |
PaulPrice
force-pushed
the
tickets/DM-11268
branch
from
e9e7827
to
b45e77b
Compare
Contains coefficients for intra-CCD crosstalk correction.
PaulPrice
force-pushed
the
tickets/DM-11268
branch
from
b45e77b
to
492486d
Compare
Contains coefficients for intra-CCD crosstalk correction.