DM-46122: Replace verify metrics for Completeness and Count based on magnitudes #237
Conversation
| @@ -5,7 +5,6 @@ description: Fully instrumented AP pipeline with fakes | |||
| imports: | |||
| - location: $AP_PIPE_DIR/pipelines/_ingredients/ApPipeWithFakes.yaml | |||
| # Most metrics should not be run with fakes, to avoid bias or contamination. | |||
| - location: $AP_VERIFY_DIR/pipelines/_ingredients/MetricsForFakes.yaml | |||
There was a problem hiding this comment.
Please rearrange the commits so that this import is removed before MetricsForFakes.yaml. Otherwise, the pipeline is in a broken state.
There was a problem hiding this comment.
It might be worth mentioning in the commit message body that the new metrics come from ApPipeWithFakes, not ap_verify. That confused me until I could trace the imports.
| apFakesCount: | ||
| class: lsst.ap.pipe.metrics.ApFakesCountMetricTask | ||
| config: | ||
| connections.metric: ApFakesCount | ||
| magMin: 1 | ||
| magMax: 39 | ||
| connections.coaddName: parameters.coaddName | ||
| connections.fakesType: parameters.fakesType |
There was a problem hiding this comment.
Did you mean to remove this task as well? I don't think we ever used it, but I don't see a counterpart in lsst/analysis_tools#286.
There was a problem hiding this comment.
This count is an output of every completeness metric now. By default we are delivering the completeness ratio (between 0 and 1) together with the total number of fakes in the bin and the number of the found fakes.
In this way, a count is redundant.
| - injectedMatch.connections.ConnectionsClass(config=injectedMatch).matchedDiaSources.name == | ||
| apFakesCountMag24t26.connections.ConnectionsClass(config=apFakesCountMag24t26).matchedFakes.name | ||
| - injectedMatch.connections.ConnectionsClass(config=injectedMatch).matchedDiaSources.name == | ||
| apFakesCount.connections.ConnectionsClass(config=apFakesCount).matchedFakes.name |
There was a problem hiding this comment.
Please do this removal on the same commit as the contracts in _ingredients. The contracts should be consistent at all times.
pipelines/HSC/ApVerifyWithFakes.yaml
Outdated
| apFakesCountMag24t26.connections.ConnectionsClass(config=apFakesCountMag24t26).matchedFakes.name | ||
| - injectedMatch.connections.ConnectionsClass(config=injectedMatch).matchedDiaSources.name == | ||
| apFakesCount.connections.ConnectionsClass(config=apFakesCount).matchedFakes.name | ||
| # contracts: |
There was a problem hiding this comment.
Please don't commit commented-out lines.
BrunoSanchez
force-pushed
the
tickets/DM-46122
branch
from
cc8c39e to
a498701
Compare
Replace metrics in MetricsForFakes.yaml for analysis_tools based metrics of completeness. New metrics come inherently inside ap_pipe ApPipeWithFakes.yaml. Delete the MetricsForFakes.yaml file and modify contracts in all pipeline yaml files to reflect this change.
BrunoSanchez
force-pushed
the
tickets/DM-46122
branch
from
a498701 to
8345e96
Compare
{Summary of changes. Prefix PR title with JIRA issue.}
sconsand/orstack-os-matrix)?ap_verify.pyon at least one of the standard datasets?For changes to metrics, the
print_metricvaluesscript fromlsst.verifywill be useful.