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

Replace localization uncertainty properties with one error radius #124

Closed
wants to merge 1 commit into from

Conversation

lpsinger
Copy link
Member

Replace the localization properties uncertainty_shape, ra_uncertainty, dec_uncertainty, and position_angle with a single property called ra_dec_error.

The original localization uncertainty properties were ill-conceived in many ways:

  1. The names ra_uncertainty and dec_uncertainty were misleading. For example, for ellipses, these fields actually represented the lengths of the semi-major and semi-minor axes, and not the uncertainty in ra or dec.

  2. The names of these properties were inconsistent with other schema. Every other uncertainty property exept for these has a name ending in _error, not _uncertainty.

  3. The rectangle shape was not useful. The most significant examples of quadrilateral localization regions are IPN error boxes. However, these are parallelograms, not rectangles, so the shape would have been under-specified.

We have observed that all current missions in GCN either report error circles or HEALPix maps. Given that observation, this patch replaces all of the ill-conceived localization properties with a single ra_dec_error field which is the radius in degrees of the error circle.

In the future, there might be a mission that requires ellipses. We won't deal with that now, but in that event we would have a few different backwards-compatible options. For example, we could:

  • Treat the ra_dec_error field as the semi-major axis, and add optional fields ra_dec_error_b and ra_dec_error_pa for the semi-minor axis and the position angle, respectively; or

  • Allow ra_dec_error to be either a single scalar, or an array of up to three numbers, representing the semi-major axis, semi-minor axis, and position angle, respectively.

Fixes #120.

Replace the localization properties `uncertainty_shape`,
`ra_uncertainty`, `dec_uncertainty`, and `position_angle` with a
single property called `ra_dec_error`.

The original localization uncertainty properties were ill-conceived
in many ways:

1. The names `ra_uncertainty` and `dec_uncertainty` were
   misleading. For example, for ellipses, these fields actually
   represented the lengths of the semi-major and semi-minor axes,
   and not the uncertainty in ra or dec.

2. The names of these properties were inconsistent with other
   schema. Every other uncertainty property exept for these has a
   name ending in `_error`, not `_uncertainty`.

3. The `rectangle` shape was not useful. The most significant
   examples of quadrilateral localization regions are IPN error
   boxes. However, these are parallelograms, not rectangles, so
   the shape would have been under-specified.

We have observed that all current missions in GCN either report
error circles or HEALPix maps. Given that observation, this patch
replaces all of the ill-conceived localization properties with
a single `ra_dec_error` field which is the radius in degrees of
the error circle.

In the future, there might be a mission that requires ellipses.
We won't deal with that now, but in that event we would have a
few different backwards-compatible options. For example, we could:

- Treat the `ra_dec_error` field as the semi-major axis, and add
  optional fields `ra_dec_error_b` and `ra_dec_error_pa` for the
  semi-minor axis and the position angle, respectively; or

- Allow `ra_dec_error` to be either a single scalar, or an array
  of up to three numbers, representing the semi-major axis,
  semi-minor axis, and position angle, respectively.

Fixes nasa-gcn#120.
@lpsinger lpsinger requested a review from jracusin February 21, 2024 18:09
@Vidushi-GitHub
Copy link
Member

I would suggest to re-think, to my knowledge, the only circular localization was limitation of the old GCN. If we believe that ellipse would be convenience for some of the users eg, GW, FRB, We should come up with creative idea during implementation, such that both the communities are happy.

@jracusin
Copy link
Contributor

I would prefer to keep the ellipse option in there so that we're planning ahead. I like @lpsinger's ra_dec_error second option with either a single scalar or an array of up to three numbers.

@lpsinger
Copy link
Member Author

I would prefer to keep the ellipse option in there so that we're planning ahead. I like @lpsinger's ra_dec_error second option with either a single scalar or an array of up to three numbers.

Alright. @Vidushi-GitHub, please update your PR accordingly.

@lpsinger lpsinger closed this Feb 21, 2024
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