New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DM-41316: Refactor overscan task for IsrTaskLSST into serial/parallel tasks. #290
Conversation
9949fd7
to
ae8e48e
Compare
This also fixes the bug so that leading/trailing rows/columns are skipped for both serial and parallel overscan according to the configs.
Note that this masking is _not_ global, because that is incorrect.
If it's configured to skip the amps, now it can.
ae8e48e
to
745ffe4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few comments. The only potential issue is the handling of crosstalk units in the future.
@@ -1014,7 +1268,7 @@ def run(self, *, ccdExposure, dnlLUT=None, bias=None, deferredChargeCalib=None, | |||
|
|||
if self.config.doVariance: | |||
# Input units: electrons | |||
self.variancePlane(ccdExposure, detector, overscans, ptc) | |||
self.variancePlane(ccdExposure, detector, serialOverscans, ptc) | |||
|
|||
if self.config.doCrosstalk: | |||
# Input units: electrons |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can't be correct. This may be an issue with the task not being up-to-date with the latest boxes, but this cannot be in electrons if the previous crosstalk correction is in ADU (as indicated above). Any CT non-linearity will make this unit difference a problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The order here is not correct according to our latest calibration boxes, that's correct. I don't think it's good for this ticket to reorder things (that's out of scope) but we should make sure that the ordering is fixed prior to/with the testing updates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DM-42459 is the ticket.
This also adds crosstalk correction in the parallel overscan region.