Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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?
GECAM_FLT and GECAM_GND conversions #35
Changes from 8 commits
22ad490
eb667a4
812852a
995b03e
1f3a946
d1309c5
26433a8
d735fce
6b26b70
605de65
737792e
873dcff
f019a07
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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?
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 byGECAM_FLT
. This line here will defeat the coverage tests forGECAM_FLT
. Please refactor the common parsing code for bothGECAM_FLT
andGECAM_GND
into a single function that is reused by both parsers.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.