-
Notifications
You must be signed in to change notification settings - Fork 24
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
WIP - Additional alert features #239
Conversation
kowalski/utils.py
Outdated
features["g-r"] = alert["candidate"]["magpsf"] - prev_alert["magpsf"] | ||
features["r-g"] = prev_alert["magpsf"] - alert["candidate"]["magpsf"] | ||
elif features["fid"] == 2: | ||
features["g-r"]["g-r"] = ( |
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.
This should just be g-r right?
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.
Nice catch. I'll fix that now.
kowalski/utils.py
Outdated
prev_alert["magpsf"] - alert["candidate"]["magpsf"] | ||
) | ||
features["r-g"] = alert["candidate"]["magpsf"] - prev_alert["magpsf"] | ||
|
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 we also do i-r and i-g to finish the permutations too?
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'm fine with that of course. Let me add it.
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.
Quick question @mcoughlin. We have g-r and r-g. Logically if we throw i in the mix it means we add r-i, i-r, g-i, and i-g.
Do we really want all of that, or do we just want one pair of each and people can just reverse the signs if they need to? So g-r, r-i, and i-g for example.
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.
@Theodlz good point, no need to have both. So g-r, r-i, and g-i (i.e. blue to red) is probably most typical.
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.
Noted. Thanks a lot.
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.
Last question @mcoughlin. How many decimals is enough? I suspect we don't need too many correct?
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.
@Theodlz yeah. 2 decimals?
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.
Sounds good. Others can always ask for another value when they look at the PR.
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.
A few suggestions.
Took care of all of them. I'd suggest taking another quick look to confirm. |
Closing, as a similar PR has been merged already |
This PR adds the structure needed for us to add features to the alert. Not used for ML (at least not for now), but useful for filtering or annotating.
In this PR, we implement the color evolution calculation, which condition to be calculated and added to the alert is: at least one detection in
r-band
and one ing-band
withinN
hours. We calculater-g
andg-r
.This
N
hour threshold can be changed in the configuration.