Skip to content
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-26593: Create an integrated pipeline for AP fake analysis #118

Merged
merged 7 commits into from Jan 7, 2021

Conversation

morriscb
Copy link
Contributor

No description provided.

Modify ap_verify pipeline
Debug connections.
Move fakes to specific pipelines.
Pull latest pipeline.

Debug pipeline.
Copy link
Member

@kfindeisen kfindeisen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, though there's a lot of code duplication that I think could be cleaned up.

Comment on lines 6 to 8
- location: $AP_PIPE_DIR/pipelines/ApPipe.yaml
- location: $AP_VERIFY_DIR/pipelines/MetricsRuntime.yaml
- location: $AP_VERIFY_DIR/pipelines/MetricsMiscWithFakes.yaml
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be more future-proof to import ApVerify.yaml here, though I see below that you end up overriding a lot of configs anyway.

pipelines/ApVerifyWithFakes.yaml Show resolved Hide resolved
Comment on lines 30 to 34
# Always prefer decorrelation; may eventually become ImageDifferenceTask default
doDecorrelation: True
detection.thresholdValue: 5.0 # needed with doDecorrelation
# Don't have source catalogs for templates
doSelectSources: False
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can leave these lines out; they're redundant with the import.

Comment on lines 40 to 41
apdb.isolation_level: READ_UNCOMMITTED
doPackageAlerts: True
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These lines are redundant if you import ApVerify.yaml, but not if you use ApPipe.yaml. (Putting the isolation level in the original pipeline was arguably a mistake, but that's out of scope for this ticket.)

pipelines/ApVerifyWithFakes.yaml Show resolved Hide resolved
Comment on lines 47 to 49
# Metric inputs must match pipeline outputs
- imageDifference.connections.coaddName == fracDiaSourcesToSciSources.connections.coaddName
- imageDifference.connections.fakesType == fracDiaSourcesToSciSources.connections.fakesType
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant if you import ApVerify.yaml, but do you need any extra contracts for the fakes* tasks?

Comment on lines 7 to 22
numNewDiaObjects:
class: lsst.ap.association.metrics.NumberNewDiaObjectsMetricTask
config:
connections.labelName: diaPipe # partial name of metadata dataset
numUnassociatedDiaObjects:
class: lsst.ap.association.metrics.NumberUnassociatedDiaObjectsMetricTask
config:
connections.labelName: diaPipe
fracUpdatedDiaObjects:
class: lsst.ap.association.metrics.FractionUpdatedDiaObjectsMetricTask
config:
connections.labelName: diaPipe
totalUnassociatedDiaObjects:
class: lsst.ap.association.metrics.TotalUnassociatedDiaObjectsMetricTask
numSciSources:
class: lsst.ip.diffim.metrics.NumberSciSourcesMetricTask
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not replace these with an import? Or (given my advice to import ApVerify.yaml above) why not create a pipeline file that only has the fakes-specific metrics, then import it and MetricsMisc.yaml separately? As it stands, the files need to be kept in sync if somebody creates a new metric.

Inherit pipelines.

Add contracts or all fakes tasks.
@morriscb morriscb merged commit e08b8c0 into master Jan 7, 2021
@morriscb morriscb deleted the tickets/DM-26593 branch January 7, 2021 00:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants