DM-42828: Add camera team crosstalk nonlinearity results

[plazas]

No description provided.

[czwa]

Some comments, and I think there are details in the application step that need checking.

[czwa]

This should initialize self.coeffsSqr as well.

[plazas]

I do that in the next line:

# Quadratic terms, if any.
        self.coeffsSqr = np.zeros(self.crosstalkShape) if self.nAmp else None
        self.coeffErrSqr = np.zeros(self.crosstalkShape) if self.nAmp else None

Are you suggesting something else?

[czwa]

Ok, I found that up in the init. It should do it here as well. It'd be annoying to init with one nAmp value, and then re-initialize everything with the length of the detector, but I think the detector is more likely to be correct, and the other entries are re-initialized here.

[czwa]

I think we do need a unit test for this. This step in particular is worrying to me. I know there are some math operations that an afw Image prevents to avoid allocating a new image in memory, and I thought multiplication like this was one of them (which is why there is scaledPlus, as that's an in-place operation).

[czwa]

If we're adding gain ratios, those also need to be applied at this step (and for the linear portion as well) if we're working in ADU.
The case for if doSqrCrosstalk and crosstalkCoeffsSqr is None should raise, but I'm not sure where that should be located. I think way down on line 904.
Finally, I don't think this method has access to self.config, as that's attached to the task, but this method is part of the calibration.

[plazas]

This step in particular is worrying to me.

Yes, I think that tImage**tImage won't work, thanks for pointing that out. Is this your only worry with this line, or do you think that the use of scalePlus here is not adequate either?

[czwa]

scalePlus should be fine, as that's an in-place operator. I think you can do one operation outside of the loop:

mi2 = mi.clone()
mi2.scaledMultiplies(1.0, mi)

and pull the squared version as tImage2 = self.extractAmp(mi2, tAmp, sAmp, isTrimmed).

[plazas]

OK, thanks, I made some changes and pushed them.

[czwa]

I'm curious about this choice.

[plazas]

I was just because the coefficients provided by Adam in this ticket are in those units. I'm happy to set the default to ADU, is that what we want? @erykoff

[erykoff]

So the proper unit name is electron not electrons. (This should be fixed elsewhere as well). Adam's crosstalk coefficients were computed after applying gain so they are in units of electrons and not ADU. However, I don't think this should necessarily be the default if unspecified (though Chris should answer if previous crosstalk matrices were generated before or after gain correction).

[erykoff]

(The proper unit name for ADU is adu)

[plazas]

@czwa, should we set the default to ADU? (adu in the code).

[plazas]

I chaged the defalt to adu. Does "proper" mean "as in astropy", in this context?

[czwa]

All existing CT has been measured pre-gain application, without flats, so I think they should be in ADU.

[czwa]

One small addition that should hopefully not cause any problems.

[czwa]

Is this a line length thing?

[plazas]

I changed it a bit now. maybe it was black?

[czwa]

Can you duplicate this test and have it use the IsrTaskLSST to get the config and the task? This should "Just Work," but I'd rather have a test that checks that the two tasks give the same results (the crosstalk task should be identical, so this is just checking that all of the config mechanics are working the same).

[plazas]

Sounds good. I had to add IsrTaskLSST to the __init__ in ip/isr in a separate commit so I could import it from the test. Tagging @aferte as a heads-up.