DM-46059: Add new metric to capture uncapped offsets between amp interfaces during ampOffsetTask execution #318
Conversation
enourbakhsh
force-pushed
the
tickets/DM-46059
branch
from
41278ee
to
0eaa7c2
Compare
| @@ -89,3 +106,29 @@ def runQuantum(self, butlerQC, inputRefs, outputRefs): | |||
| raise NoWorkFound(f"No metadata entries for {taskName}.") | |||
| outputs = self.run(data=metadata, plotInfo=plotInfo) | |||
| butlerQC.put(outputs, outputRefs) | |||
|
|
|||
|
|
|||
| class DictTypeDatasetMetadataAnalysisTask(AnalysisPipelineTask): | |||
There was a problem hiding this comment.
Should we simply call this DatasetMetadataAnalysisTask, or do we expect there to be other, non-dict datasets?
There was a problem hiding this comment.
We also expect non-dict metadata that simply offer a value for each metric name; I can add one for such regular 1:1 mappings for completeness. This specific ticket focuses on the interactions between many amplifiers on a detector, so it made sense to store information about all those so-called interface offsets in a dictionary that pairs amp names with the offsets, rather than cluttering our metadata with less human-readable names.
There was a problem hiding this comment.
Introduced the DatasetMetadataAnalysisTask class, which adapts its behavior based on the metricsStoredAsDict config option.
There was a problem hiding this comment.
In that case, do feel free to revert back to DictTypeDatasetMetadataAnalysisTask and remove the metricsStoredAsDict parameter. I'll leave that up to you, though.
There was a problem hiding this comment.
I think having it this way with metricsStoredAsDict is more future-proof because most metrics I know are not stored as dictionaries.
enourbakhsh
force-pushed
the
tickets/DM-46059
branch
from
8037a9e
to
8dc3992
Compare
pipelines/calexpMetrics.yaml
Outdated
| metadataDimensions: ["instrument", "visit", "detector"] | ||
| atools.calexpMetadataMetrics: MetadataMetricTool | ||
| connections.inputName: calibrate_metadata | ||
| connections.outputName: calibrate_metadata_analysis |
There was a problem hiding this comment.
Could you remove _analysis from this output name, please? While I had thought it was a problem, it turns out it isn't, as _metrics is added to the end of the output name - so what we'd end up with here is calibrate_metadata_analysis_metrics. I'd rather keep the names as short as possible, while still being meaningful and unique. Might be worth adding a # Appended with "_metrics" - or similar - comment here so I don't get confused again!
Sorry for the confusion!
pipelines/pviMetrics.yaml
Outdated
| metadataDimensions: ["instrument", "visit", "detector"] | ||
| atools.calibrateImageMetadataMetrics: MetadataMetricTool | ||
| connections.inputName: calibrateImage_metadata | ||
| connections.outputName: calibrateImage_metadata_analysis |
There was a problem hiding this comment.
Same applies here (i.e., please remove _analysis).
pipelines/visitQualityCore.yaml
Outdated
| metadataDimensions: ["instrument", "visit", "detector"] | ||
| atools.calexpMetadataMetrics: MetadataMetricTool | ||
| connections.inputName: calibrate_metadata | ||
| connections.outputName: calibrate_metadata_analysis |
| dimensions=frozenset(config.metadataDimensions), | ||
| ) | ||
|
|
||
| self.dimensions = frozenset(config.inputDimensions) |
There was a problem hiding this comment.
self.dimensions = frozenset(config.inputDimensions) should come before super().__init__(config=config) so that the inputDimensions are picked-up here, when the metrics output connection is defined. Otherwise, the output metric bundle doesn't have any dimensions!
I should have realised this when I asked you to set dimensions={} above - sorry for my oversight.
enourbakhsh
force-pushed
the
tickets/DM-46059
branch
from
5533deb
to
20a34cc
Compare
No description provided.