-
Notifications
You must be signed in to change notification settings - Fork 22
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
Adding count rate in core #127
Conversation
Co-authored-by: Leo Singer <[email protected]>
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, but I'd at least like @jracusin to also review this.
@jracusin please check this PR. |
@@ -9,6 +9,14 @@ | |||
"type": "number", | |||
"description": "False alarm rate: the rate of occurrence of non-astrophysical events that are of the same intensity or significance as the current event [Hz]" | |||
}, | |||
"net_count_rate": { | |||
"type": "number", | |||
"description": "Net count rate of the transient above the background [counts/s]" |
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.
The description should specify that this is over the rate_duration and rate_energy_range.
}, | ||
"backgound_count_rate": { | ||
"type": "number", | ||
"description": "Count rate of the background for the transient [counts/s]" |
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.
Same comment about rate_duration and rate_energy_range as in the net_count_rate.
}, | ||
"backgound_count_rate": { | ||
"type": "number", | ||
"description": "Count rate of the background for the transient [counts/s] over rate_duration and rate_energy_range." |
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.
"description": "Count rate of the background for the transient [counts/s] over rate_duration and rate_energy_range." | |
"description": "Count rate of the background for the transient [counts/s] over backgound rate duration and rate energy range." |
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.
@jracusin we can't use same rate_duration property for the background. However we can add background time range array.
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.
The background rate comes from using multiple intervals, but it is used to estimate the background during the source interval. I think this use is fine.
"backgound_count_rate": { | ||
"type": "number", | ||
"description": "Count rate of the background for the transient [counts/s] over rate_duration and rate_energy_range." | ||
}, |
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 do not think we need to include this.
@jracusin background time intervals removed. Is everything else approved? |
9119fdc
to
e84f4da
Compare
Rebasing had some issues and we should notify these changes to Einstein Probe soon, so opened a new PR #136 and closing this. |
One property of the Einstein Probe is useful for various missions - net count rate.
However, it requires a consistent definition for other teams as well.
Example, some teams report count/s with background subtraction, others report count rates with background.
To ensure consistency, it would be beneficial for the GCN team to standardize this property.