Skip to content
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

SWIFT_BAT_GRB_POS_ACK conversion #36

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

athish-thiru
Copy link
Contributor

All SWIFT_BAT_* notices have a very similar similar conversion with just a few differing/missing fields. So if the format is acceptable I could commit the rest of the conversions to this pull request as well.

The following keywords do not exist in the core schema for SWIFT_BAT_GRB_POS_ACK:

  • n_events
  • image_peak
  • bankground_events
  • background_start_time
  • background_duration
  • trigger_index
  • catalog_number

Copy link
Member

@lpsinger lpsinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove the xfail mark for the unit test.

@lpsinger
Copy link
Member

lpsinger commented Aug 7, 2024

@parsotat, would you please review this?

…sary equalities

changed Unused to intentionally omitted

reformated comments
@athish-thiru athish-thiru requested a review from lpsinger August 19, 2024 16:19
Copy link
Member

@lpsinger lpsinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did @parsotat ever take a look at this?

Comment on lines 43 to 45
comments = "\n".join(
[val for (key, val) in flag_descriptions.items() if (soln_status_bits[key])]
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please capture these flags in a machine-readable way, not as comments.

Copy link
Contributor Author

@athish-thiru athish-thiru Aug 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I use additional_info to capture the data stored in the COMMENTS: fields in the text notices. Some of these are redundant with the data in the notices and can be removed(I've mainly added them for completeness). However, some of the these comments I think may be too complex to easily translate into a field like "StarTracker not locked so trigger probably bogus" or "This was orginally a SubTresh, but it is now converted to a real BAT_POS.".

Copy link

@bob-wiegand bob-wiegand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ran a spell checker and noticed some typos

"image_peak": bin[10],
"background_events": bin[22],
"background_start_time": utils.datetime_to_iso8601(bin[5], bin[23]),
"backgroun_duration": bin[24] * 1e-2,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: should be "background_duration"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @bob-wiegand! Could you link the package that you used for spell-checking? I think it would be easier for me to run this on my end and correct it for each notice type instead of you having to manually list each error. Thanks!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: I am not sure it would be simpler since it was still somewhat manual.
I ran https://github.com/crate-ci/typos
$ typos --words
on the files to find the words then
$ hunspell -d en_US
to check them. hunspell will build up a dictionary of excusable terms for gcn-*.

The silliest aspect is that I was unable to find a way to get typos to exclude times (e.g., 1987-01-02T03:04:05.6); although extend-ignore-re will exclude the times from the misspelling list, I could not figure out how to get it to exclude them from the word/identifier list.

"image_peak": bin[10],
"background_events": bin[22],
"background_start_time": utils.datetime_to_iso8601(bin[5], bin[23]),
"backgroun_duration": bin[24] * 1e-2,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: background_duration again

"image_peak": bin[10],
"background_events": bin[22],
"background_start_time": utils.datetime_to_iso8601(bin[5], bin[23]),
"backgroun_duration": bin[24] * 1e-2,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: background_duration again

else:
grb_status = "It is not a GRB"

calalog_num = bin[25]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does not affect the output, but suggest renaming variable: s/calalog/catalog/

else:
grb_status = "It is not a GRB"

calalog_num = bin[25]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo on catalog

"flaring_known_source": bool(soln_status_bits[2]),
"star_tracker_status": start_tracker_status[soln_status_bits[10]],
"bright_star_nearby": bool(soln_status_bits[13]),
"originally_subtresh": bool(soln_status_bits[14]),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

update subtresh to subthresh.
Would "was_subthreshold" be a better name?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants