-
Notifications
You must be signed in to change notification settings - Fork 5
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
GECAM_FLT and GECAM_GND conversions #35
base: main
Are you sure you want to change the base?
Conversation
Explicit int conversion removed MAXI_UNKNOWN conversion Removed GECAM_FLT and GECAM_GND conversion Added documentation and changed function name reformatted file deleted files
bin[ | ||
11 | ||
] # Temporarily Unused. Should have a int->class_type mapping but incomplete documentation |
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, would you please help @athish-thiru find the integer to class name mapping for this field?
bin[ | ||
18 | ||
] # Temporarily Unused. Should have a bit->flag mapping but incomplete documenation |
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 need help with interpreting this field too.
bin[ | ||
26 | ||
] # Temporarily Unused. Presumably which detectors activated but incomplete documenation |
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 need help with this field.
"trigger_time": utils.datetime_to_iso8601(bin[5], bin[6] * 1e-2), | ||
"trigger_type": "rate", | ||
"rate_energy_range": [bin[24], bin[25]], | ||
"rate_snr": bin[20] * 1e-2, |
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 field is called trig_signif
. Do we know that this is equivalent to SNR?
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.
It isn't documented, so we can't be completely certain. I was mainly going of the text notices which had the following:
TRIGGER_SIGNIF: 5.1 [sigma] (Signif of the Trigger)
Linked here
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.
It's not documented, but we do have the source code. Also we can contact the GECAM mission.
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.
According to the source code: "Height of the triggering peak"
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.
That could mean SNR if it is the height of an SNR time series, but it could equally well be the height of the the count rate time series. @Vidushi-GitHub, @jracusin, some help here please.
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.
It looks a best match with rate_snr in Statistics schema. Most likely height of the peak value from the light curve i.e. counts/s or rate.
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.
It doesn't sound like we reached a definite answer on this yet. @Vidushi-GitHub, how would you like to resolve this?
…ted containment probability
|
||
soln_status_bits = np.flip(np.unpackbits(bin[18:19].view(dtype="u1"))) | ||
|
||
containment_prob = norm().cdf(1) - norm.cdf(-1) |
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.
Lift to module scope.
I added a trigger_duration field which is not in the core schema. |
soln_status_bits = np.flip(np.unpackbits(bin[18:19].view(dtype="u1"))) | ||
comments = None | ||
if soln_status_bits[0] == 1: | ||
comments = "This is a test notice." |
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.
We should capture this in a machine-readable way, not as a comment.
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 can add 'alert_tense' for this, should I remove the comment?
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.
|
||
def parse(bin): | ||
bin[15] # Unused. According to docs: '4 bytes for the future' | ||
bin[20:39] # Unused. According to docs: '76 bytes for the future' |
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.
These fields are unused by GECAM_GND
, but some of them are used by GECAM_FLT
. This line here will defeat the coverage tests for GECAM_FLT
. Please refactor the common parsing code for both GECAM_FLT
and GECAM_GND
into a single function that is reused by both parsers.
def parse(bin): | ||
bin[15] # Unused. According to docs: '4 bytes for the future' | ||
bin[27:39] # Unused. According to docs: '32 bytes for the future' | ||
bin[21] # Temporarily Unused. This is trigger_duration; currently no keyword |
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.
Please work with @Vidushi-GitHub to create a schema entry for this field.
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.
My bad, I should have removed this line! After speaking with @Vidushi-GitHub, we decided to name this trigger_duration i.e. keep the name from the source code which is present in the return statement.
Conversion is not finished as there is incomplete documentation for binary packets.
Additionally, there seems to be an incorrect encoding for trig_sod as it needs to be divided by an another factor of 100 to get the correct datetime.