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-30171: Define PTC tests for cp_verify #18
Conversation
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 style notes, but I don't think there's any major issues.
python/lsst/cp/verify/verifyCalib.py
Outdated
@@ -86,15 +96,33 @@ class CpVerifyCalibTask(pipeBase.PipelineTask, pipeBase.CmdLineTask): | |||
ConfigClass = CpVerifyCalibConfig | |||
_DefaultName = 'cpVerifyCalib' | |||
|
|||
def run(self, inputCalib): | |||
def runQuantum(self, butlerQC, inputRefs, outputRefs): |
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 runQuantum
doesn't do anything that the default wouldn't, so it doesn't need to be here.
@@ -73,7 +73,7 @@ def detectorStatistics(self, inputCalib): | |||
|
|||
return outputStatistics | |||
|
|||
def verify(self, calib, statisticsDict): | |||
def verify(self, calib, statisticsDict, **kwargs): |
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 should match the signature as it is in verifyCalib.py
.
if not testBool: | ||
verifyDetStatsFinal[testName] = bool(np.all(list(verifyDetStats[testName]))) | ||
|
||
return verifyDetStatsFinal, bool(success) |
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.
Github is complaining about the cases of the booleans in the VerifyDefect.yaml pipeline definition, but doesn't let me put a comment there, so it's here instead.
outputStatistics : `dict` [`str`, scalar] | ||
A dictionary of the statistics measured and their values. | ||
""" | ||
return {} |
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.
Can this be used to store the values that are being tested? I know this duplicates information from the PTC dataset, but it will make it easier to migrate that information into a database in the future.
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.
Yes. Is there an example of this in other "verify" tests? Should they be called something like "PTC_GAIN", "PTC_NOISE", etc. ?
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 thinking about putting these in a new amplifierStatistics
method since the relevant quantities are per amp (gain, turnoff, noise, a00).
Remove amp function in verify PTC
f75f374
to
b4f6bbd
Compare
No description provided.