DM-30169: Define flat tests for cp_verify #5
Conversation
pipelines/VerifyDefectPostFlat.yaml
Outdated
| connections.ccdExposure: 'raw' | ||
| connections.outputExposure: 'verifyDefectProc' | ||
| overscan.fitType: 'MEDIAN_PER_ROW' | ||
| doWrite: True |
There was a problem hiding this comment.
I believe once Nate told me that booleans in the YAML scripts should start with lowercase. I'm not sure if there's anything official in the guide (I think not), and I might have seen both conventions, so I'll leave it at your discretion :)
There was a problem hiding this comment.
Yes, if you run yamllint the default is to prefer true for this. Consider adding the yamllint GitHub Action.
There was a problem hiding this comment.
I've added yamllint, pushed, and after fixing things up, it seems to be working. I'll file a ticket to do the same for cp_pipe, as those pipelines were also written by me, and almost certainly have capital letters.
| @@ -128,11 +128,14 @@ def run(self, inputStats, camera, inputDims): | |||
| outputStats = {} | |||
| success = True | |||
|
|
|||
| mergedStats = {} | |||
There was a problem hiding this comment.
Are you merging the metrics/stats from all detectors to do an overall statistics test? Or what is the purpose of the merging?
There was a problem hiding this comment.
The goal of the merge is to allow there to be a hierarchical path from a single top level file (the output of CpVerifyRunMergeTask) that contains a list of all the failures, through the exposure level down to the detector/amp level. The visualization notebooks (which I still need to extend for multi-detector cameras) can start with the top level file, and use that to determine what needs to be examined because of the failure.
| for ampName, stats in ampStats.items(): | ||
| verify = {} | ||
|
|
||
| # DMTN-101 Test 10.X: confirm that per-amplifier scatter is |
There was a problem hiding this comment.
Am I looking at the wrong version of the DMTN-101 document? https://dmtn-101.lsst.io/ I can't see the definition of the tests that you implement here under section 10.
There was a problem hiding this comment.
10.X was my shorthand for "this will be defined in section 10, but isn't formally done yet." I want to get the code written first, because adding/updating tests is easy once the code exists, and getting something running is easier than getting DMTN-101 updates approved.
python/lsst/cp/verify/verifyFlat.py
Outdated
| ---------- | ||
| exposure : `lsst.afw.image.Exposure` | ||
| The exposure the statistics are from. | ||
| statisticsDictionary : `dict` [`str`, `dict` [`str`, scalar]], |
There was a problem hiding this comment.
The name statisticsDictionary is different from statisticsDict in the argument.
There was a problem hiding this comment.
Fixed here and elsewhere.
| detStats = statisticsDict['DET'] | ||
|
|
||
| # DMTN-101 Test 10.Y: confirm intra-chip scatter is small. | ||
| verifyDet['SCATTER'] = bool(detStats['SCATTER']/detStats['MEAN'] <= 0.05) |
There was a problem hiding this comment.
This definition of "small" (0.05) is not expected to be modified? Just pointing that out in case that we want to make it a config parameter.
There was a problem hiding this comment.
I've been working with the assumption that once we finish DMTN-101, the tests will be fixed; those will be the criteria we use for calibrations.
| The exposure the statistics are from. | ||
| statisticsDictionary : `dict` [`str`, `dict` [`str`, scalar]], | ||
| Dictionary of measured statistics. The inner dictionary | ||
| should have keys that are statistic names (`str`) with |
There was a problem hiding this comment.
Maybe put some examples of the expected statistics names, e.g., "SCATTER", "MEAN", etc.?
There was a problem hiding this comment.
The names are somewhat arbitrary. I didn't want to force the name to equal the afwStatistics string name, as I know there will be metrics that are not calculated from afwStatistics (brighter-fatter will have a size-flux slope as an example).
| # Get detector stats: | ||
| detectorMeans.append(stats['DET']['MEAN']) | ||
|
|
||
| return {'SCATTER': np.stdev(detectorMeans)} |
There was a problem hiding this comment.
why do we use NumPy and not afw? It doesn't matter?
There was a problem hiding this comment.
I didn't think there'd be any difference. This is a straight standard deviation, so they should be algorithmically identical (no clipping or masking needed).
| success = True | ||
|
|
||
| # DMTN-101 Test 10.Z: confirm inter-chip scatter is small. | ||
| verifyStats['SCATTER'] = bool(statisticsDictionary['EXP']['SCATTER'] <= 0.05) |
There was a problem hiding this comment.
Same comment as above about hard-coded 0.05.
This defines an initial set of flat verification tests:
Additional helper code is added, as well as the ability to measure per-exposure statistics.