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-40136: Add AA1 metric #178
Conversation
76fc760
to
ce6e855
Compare
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.
Would it make sense to also calculate a version of AA1 that is the total positional offset? That could be useful to have sometimes.
Also, does it make sense to apply both S/N cuts (from VisitContext) and mag cuts (the “brightStar” selection)? I would think we want one or the other, but not both.
I place a few comments here, but I think we should discuss the implementation of AB1 and ABF1 on the Jira ticket. I'll post questions there.
@@ -153,6 +153,7 @@ class FracThreshold(ScalarFromVectorAction): | |||
threshold = Field[float](doc="Threshold to apply.") | |||
percent = Field[bool](doc="Express result as percentage", default=False) | |||
relative_to_median = Field[bool](doc="Calculate threshold relative to " "the median?", default=False) | |||
absolute_value = Field[bool](doc="Take absolute value of distribution") |
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.
Should this have a default? (i.e., default=False
?)
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.
Yes, but I'm moving it to DM-39256 anyway.
) | ||
|
||
self.process.calculateActions.AA1 = MedianAction(vectorKey="brightStars") | ||
self.process.calculateActions.AB1 = SigmaMadAction(vectorKey="brightStars") |
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 don't think this is what AB1 is meant to be. It should be the difference between separations measured in the r-band and those measured in any other filter. This seems to be calculating only the RMS of the separations in the input visit's filter?
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.
Renaming AB1 to AA1_sigmaMad.
self.process.calculateActions.ABF1 = FracThreshold( | ||
vectorKey="brightStars", | ||
threshold=self.AB2, | ||
percent=True, | ||
op="gt", | ||
relative_to_median=True, | ||
absolute_value=True, | ||
) |
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 also think this should be calculated using separations in the visit's band relative to separations in the r-band. (i.e., it requires a pair of visits -- one in a non-r band, and one in r-band)
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.
Moved to DM-39256.
71787a1
to
1e9c3a6
Compare
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.
Looks good -- approved!
} | ||
|
||
def visitContext(self) -> None: | ||
"""Apply visit options for the metrics. Applies the coadd plot flag |
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.
"coadd plot flag" --> "visit plot flag"?
|
||
|
||
class TargetRefCatDeltaMetrics(AnalysisTool): | ||
"""Calculate the AA1, AB1, and ABF1 metrics from the difference between the |
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 think it's only calculating AA1 now?
1e9c3a6
to
f95ccbe
Compare
No description provided.