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-43231: Create Analysis Tool to send visitSummary info to sasquatch #906
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.
Minor comments
@@ -46,6 +46,16 @@ | |||
from .computeExposureSummaryStats import ComputeExposureSummaryStatsTask | |||
|
|||
|
|||
class _EmptyTargetTask(pipeBase.PipelineTask): |
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 would still add a doctoring to this to indicate it is only a placeholder, why, and that no one should use it (I know the _ is supposed to mean that, but more documentation is always better)
storageClass="MetricMeasurementBundle", | ||
dimensions=("instrument", "visit", "detector"), | ||
) | ||
|
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 would write out the SummaryStats too if someone wants to create the metrics later. I don't know why we already don't, maybe @erykoff knows. Is it just because we ARE writing it out with the exposure?
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 actually confused about this whole thing. You can get the summary stats as a component of a calexp with calexp.summaryStats
in a connection without reading the full calexp. I'm not sure why this code is in calibrate.py
at all (beyond the circular import problems). Furthermore, we are moving from calibrate.py
to calibrateImage.py
(which is already the default in AP) but I don't think this belongs there either.
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.
It's so that the SummaryStats are written to metrics during the calibrate task, which I understand is something that both @ctslater and @mfisherlevine would like. I agree that grabbing the stats is easy and quick post-calibrate, but it still involves read overhead, which is avoided by having it in-task like this. AFAIA, it's always been a feature of analysis_tools that they can be used in-task like this.
The reason it's in calibrate.py rather than calibrateImage.py is simply because this is where I started developing it. It can be easily added to calibrateImage.py, and I think @ctslater is planning to make a ticket to do that.
f16a721
to
f088b23
Compare
Added the ability to create and write metrics from the calexp summary stats. The task to calculate the metrics must be retargeted at runtime to avoid circular imports in calibrate. This is the reason for the private _EmptyTargetTask.
9cfd570
to
e25e93e
Compare
e25e93e
to
6a7085f
Compare
Added the ability to create and write metrics from the calexp summary stats. The task to calculate the metrics must be retargeted at runtime to avoid circular imports in calibrate. This is the reason for the private _EmptyTargetTask. This gets retargeted to CalexpSummaryTask in analysis tools, which is what creates the MetricMeasurementBundle that gets written to sasquatch.