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-41108: Temporarily remove broken AP metrics #218

Merged
merged 6 commits into from Mar 15, 2024
Merged

Conversation

isullivan
Copy link
Contributor

To be added back in DM-43201

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.

No concerns about the change itself, but I'd like to see some safeguards to make sure these metrics stay fast and slim.

Copy link
Member

Choose a reason for hiding this comment

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

Neither apDetectorVisitQualityCore nor "Tier 1 AP plots and metrics to assess CcdVisit quality" really makes it clear how these metrics will be used.

Is the plan that this file can only contain metrics that wouldn't be disruptive to real-time processing? If so, a warning about the restriction should be present in the name, description, or both. (I'm assuming, of course, that there'd be a bigger and slower set of metrics somewhere for batch or daytime processing...)

@isullivan isullivan force-pushed the tickets/DM-41108 branch 4 times, most recently from d8a7b1e to f38f02e Compare March 13, 2024 23:57
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, thanks for the clean commit history!

@@ -1,6 +1,9 @@
description: |
AP plots and metrics to assess CcdVisit quality in Prompt Processing
Additional AP metrics may be written in apDetectorVisitQualityExtended
parameters:
coaddName: goodSeeing
fakesType: ''
Copy link
Member

Choose a reason for hiding this comment

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

Does this need a comment saying that the parameter names are shared with ApPipe (I assume that's the intent)?

@isullivan isullivan merged commit 80a0569 into main Mar 15, 2024
8 checks passed
@isullivan isullivan deleted the tickets/DM-41108 branch March 15, 2024 19:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants