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-23983: Cannot apply crosstalk in Gen 3 DECam processing #140
Conversation
|
||
return Struct( | ||
coeff=coeff, | ||
coeffErr=coeffErr, | ||
coeffNum=coeffNum | ||
coeffNum=coeffNum, | ||
calib=calib, |
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.
Please add this returned value to the docstring, and while you're at it, maybe make the summary something moderately more informative than "implement scatter/gather" because I don't know what that means.
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.
Done. I've also noted that this method will disappear in DM-24760, as the current logic is confusing at best.
tableList.append(newTable) | ||
extNum += 1 |
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.
Does this actually change any behavior?
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 with warnings.catch_warnings("error")
is needed because astropy will read the "first available" table if the requested table does not exist, throwing a warning instead of an exception. This promotes the warning to an exception, and ensures that the list represents what's in the FITS table.
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.
Wow, that is subtle. Sounds good!
kwargs : | ||
Other keyword parameters to set in the metadata. |
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.
Should this be type dict
? I've never seen kwargs documented quite like this.
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.
Updated with dict
and collections.abc.Mappings
, based on the upstream version: https://github.com/lsst/daf_base/blob/master/python/lsst/daf/base/propertyContainer/propertyContainerContinued.py#L469.
This new implementation supports both intra-chip crosstalk via the CrosstalkCalib.coeffs attribute and inter-chip crosstalk via the CrosstalkCalib.interChip dictionary. Generic methods have been moved into the calibration, and the CrosstalkTask.run updated to work with the new calibration and to apply the inter-chip crosstalk if it exists.
Simplify IsrMock as the calibration handles amplifier flips. Update measureCrosstalk to persist output as CrosstalkCalib, and fix broken unit tests.
Update output type to a generic Exposure needed for running pipelines that split ISR processing into separate task calls (such as DECam ISR with Crosstalk).
DM-25348 will resolve this problem.
Implement CrosstalkCalib class inheriting from IsrCalib.
Add inter-chip crosstalk handling.
Switch to Exposure (no F) as the output product in gen3 to allow IsrTask to process an input in multiple passes.