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-43404: Add diffim QA plots #231

Merged
merged 13 commits into from Apr 5, 2024
Merged

DM-43404: Add diffim QA plots #231

merged 13 commits into from Apr 5, 2024

Conversation

isullivan
Copy link
Contributor

No description provided.

@isullivan isullivan force-pushed the tickets/DM-43404 branch 4 times, most recently from 1a29b89 to 9d27563 Compare March 27, 2024 23:03
Copy link
Contributor

@ebellm ebellm left a comment

Choose a reason for hiding this comment

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

Some worries about naming

@@ -6,6 +6,7 @@
from .cpVerifyQuantityProfile import *
from .deblenderMetric import *
from .deltaSkyCorr import *
from .diaMetricsPlots import *
Copy link
Contributor

Choose a reason for hiding this comment

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

The name here is confusing, because "metrics" here doesn't mean analysis_tools metrics, but the metrics stored in source catalogs. Is there a way we can make this more clear?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I renamed the module to diaSpatialMetricsPlots. Does that help? I could also go with diaSpatiallySampledMetricsPlots



class DiffimMetricsInterpolatePlot(AnalysisTool):
"""Interpolate metric values evaluated at locations in a supplied catalog
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, I'm worried about overloading the word "metric" here to mean very different software representations. (You could make an analysis_tools metric from these "metrics", for instance.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed the wording.

@isullivan isullivan force-pushed the tickets/DM-43404 branch 3 times, most recently from 7097f63 to b54de91 Compare April 2, 2024 03:52
pass


class DiffimDetectorVisitPlotsAnalysisTask(AnalysisPipelineTask):
Copy link
Contributor

Choose a reason for hiding this comment

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

Apologies, I think this probably needs a rename as well, since it only takes the spatially sampled metrics as input (rather than something more general to diffim.) DiffimDetectorVisitSpatiallySampledPlots?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll go with DiffimDetectorVisitSpatiallySampledMetricsTask, to match the dataset name.

@isullivan isullivan force-pushed the tickets/DM-43404 branch 3 times, most recently from 5f5abe6 to 65f4d1e Compare April 4, 2024 22:36
@isullivan isullivan merged commit 55162e8 into main Apr 5, 2024
8 checks passed
@isullivan isullivan deleted the tickets/DM-43404 branch April 5, 2024 03:39
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