-
Notifications
You must be signed in to change notification settings - Fork 14
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-6147 #13
DM-6147 #13
Conversation
@@ -619,7 +625,7 @@ def getIsrExposure(self, dataRef, datasetType, immediate=True): | |||
return exp | |||
|
|||
def saturationDetection(self, exposure, amp): | |||
"""!Detect saturated pixels and mask them using mask plane "SAT", in place | |||
"""!Detect saturated pixels and mask them using mask plane config.saturatedMaskName, in place |
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 like this change!
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.
Thanks. I'm not convinced this should be a config param, but since it is, the old docs were wrong.
I feel like there should be a test that does both saturation and suspect detection, to help define how those two interact. Whether that should live in testSuspectMasking.py or some other file, I'm not sure, but I feel like it's an important integration test. It would call isrTask.run(), but maybe that suggests that the stuff inside the "for amp in ccd:" should live in a "do_amp()" method, which can then be tested separately? |
I would like to leave that for DM-6161 if at all possible. We don't presently have any unit test for IsrTask and we really need one. |
Ok: glad there's a ticket about it. Should I (or you?) add my note above about the possible refactor to that ticket? |
I don't understand about the refactoring. Perhaps you should add the note or explain it to me in person. |
No description provided.