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-12985: Interpolate before applying B-F correction. #104
Conversation
python/lsst/obs/subaru/isr.py
Outdated
self.maskAndInterpNan(ccdExposure) | ||
interpolationDone = True | ||
|
||
# Undo flat and dark application |
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.
Doing and then undoing suggests using a context manager to simplify the main code path.
|
||
if self.config.doDefect: | ||
if self.config.doDefect and not interpolationDone: | ||
self.maskAndInterpDefect(ccdExposure, defects) |
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.
Why leave this here?
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.
Where would I move it instead? I think it's the right place to do defect interpolation if you're not running brighter-fatter (or at least one of several right places, and it's the old one).
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.
It took me a while to figure out that not interpolationDone
effectively means not self.config.doBrighterFatter
. In that case, I think you're right.
python/lsst/obs/subaru/isr.py
Outdated
self.saturationInterpolation(ccdExposure) | ||
|
||
if self.config.doFringe: | ||
self.fringe.runDataRef(ccdExposure, sensorRef) | ||
if self.config.doSetBadRegions: | ||
self.setBadRegions(ccdExposure) | ||
|
||
self.maskAndInterpNan(ccdExposure) | ||
if not interpolationDone: | ||
self.maskAndInterpNan(ccdExposure) |
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.
I think we want this to fire always --- there might be zeros in the flat.
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.
I'm planning to go with @RobertLuptonTheGood's proposal to make this a config option but disable it by default.
I do not want to test for 0s in the flat; that's part of the calibs QA.
|
I don't want to test for it either, but I think it's important we can deal with any |
I'd leave it in as an option but not run it. It's easy to lose 0.5% here and 1% there. We could have an option to assert on !NaN
|
Interpolation can only be done on flat-fielded images, so we apply (and later remove) both that and the dark (for good measure) before running B-F.
1283469
to
34a4194
Compare
DM-12985: Interpolate before applying B-F correction.
Interpolation can only be done on flat-fielded images, so we apply
(and later remove) both that and the dark (for good measure) before
running B-F.