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-41118: Add metrics from diffim task metadata #154
Conversation
9af11d3
to
baea6e9
Compare
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.
There are a few suggestions to change things up, what path you choose is up to you.
|
||
from ..actions.scalar import MedianAction | ||
from ..interfaces import AnalysisTool | ||
|
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.
There is a lot of duplication here, I'm going to make some suggestions that make use of Analysis_Tools designed configurability.
Start by deleting all of the metrics below and replace with one tool:
# Pick whatever name you want
class DiffimMetadataMetricTool(AnalysisTool):
"""This tool is designed to calculate the median of data produced from diffim metric data"""
# This is a convenience field to make it easier to set things from the "top level"
# you could also just reach right in and set things in config as well and leave
# this out.
metricKey = Field[str](doc="The metadata field to process, name will be used as the metric name")
units = Field[str](doc="The units to associate with the metric")
def setDefaults(self):
self.process.calculateActions.diffimMedian = MedianAction(vectorKey="")
self.produce.metric.units = {"diffimMedian": "ct"}
def finalize(self):
# This function is implemented to support the convenience fields defined above
self.process.calculateActions.diffimMedian.vectorKey = self.metricKey
self.produce.metric.units["diffimMedian"] = self.units
self.produce.metric.newNames = {"diffimMedian": self.metricKey}
Alternatively, we could build a tool that takes a configuration of a dict field for all the metrics to produce with one tool
# Pick whatever name you want
class DiffimMetadataMetricTool(AnalysisTool):
"""This tool is designed to calculate the median of data produced from diffim metric data"""
metrics = DictField[str, str](doc="The metrics to calculate from the task metadata and their units")
def finalize(self):
for name, unit in self.metrics.items():
setattr(self.process.calculateActions, name, MedianAction(vectorKey=name))
self.produce.metric.units = dict(self.metrics.items())
In this case the yaml would change to look something like:
atools.diffimMetadataMetric: DiffimMetaDatametricTool
atools.diffimMetadatametric.metrics:
nMergedDiaSources: ct
nGoodPixels: ct
...
This would result in a single atools bundle uploaded to sasquatch with multiple measurements inside it. Both are perfectly reasonable ways to do things, and the dashboards can handle plotting from each the same way. The big difference is that if you are getting these back from the butler, the former will result in n dataset types and n butler.get
calls. The later will be a single dataset type and a single butler.get
call with multiple values in it.
class: lsst.analysis.tools.tasks.DiffimDetectorVisitAnalysisTask | ||
config: | ||
connections.outputName: diffimMetadata | ||
atools.nUnmergedDiaSources: NUnmergedDiaSourcesMetric |
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.
atools.nUnmergedDiaSources: DiffimMetadataMetricTool
atools.nUnmergedDiaSources.metricKey: nUnmergedDiaSources
atools.nUnmergedDiaSources.units: ct
config: | ||
connections.outputName: diffimMetadata | ||
atools.nUnmergedDiaSources: NUnmergedDiaSourcesMetric | ||
atools.nMergedDiaSources: NMergedDiaSourcesMetric |
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.
atools.nUnmergedDiaSources: DiffimMetadataMetricTool
atools.nUnmergedDiaSources.metricKey: nMergedDiaSources
atools.nUnmergedDiaSources.units: ct
connections.outputName: diffimMetadata | ||
atools.nUnmergedDiaSources: NUnmergedDiaSourcesMetric | ||
atools.nMergedDiaSources: NMergedDiaSourcesMetric | ||
atools.nGoodPixelsMetric: NGoodPixelsMetric |
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.
atools.nUnmergedDiaSources: DiffimMetadataMetricTool
atools.nUnmergedDiaSources.metricKey: nGoodPixel
atools.nUnmergedDiaSources.units: ct
atools.nUnmergedDiaSources: NUnmergedDiaSourcesMetric | ||
atools.nMergedDiaSources: NMergedDiaSourcesMetric | ||
atools.nGoodPixelsMetric: NGoodPixelsMetric | ||
atools.nBadPixelsMetric: NBadPixelsMetric |
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.
similar to above for each of the tools below
atools.scaleTemplateVariance: TemplateVarianceScaleMetric | ||
atools.templateCoverage: TemplateCoverageMetric | ||
python: | | ||
from lsst.analysis.tools.atools import * |
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.
Probably best practice to import your module here instead of star
@@ -12,3 +12,22 @@ tasks: | |||
atools.simpleSky: SimpleDiaPlot | |||
python: | | |||
from lsst.analysis.tools.atools import * |
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.
The changes below will make sense after reading the metric definition comments.
class DiffimDetectorVisitAnalysisConnections( | ||
AnalysisBaseConnections, | ||
dimensions=("visit", "band", "detector"), | ||
defaultTemplates={"coaddName": "goodSeeing", "fakesType": "fakes_"}, |
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 confused on why these templates are here?
b7ece7e
to
5a3fce8
Compare
5a3fce8
to
12dfefd
Compare
This abbreviation will cause an error if any metric uses the 'ct' unit, which can be hard to diagnose.
12dfefd
to
55295de
Compare
No description provided.