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

NEOS FOV State visibility checks are not performing frame conversions #155

Open
dahlend opened this issue Nov 14, 2024 · 2 comments
Open
Assignees
Labels
backend Issue in the rust backend bug Something isn't working

Comments

@dahlend
Copy link
Collaborator

dahlend commented Nov 14, 2024

Bug

FOVs which are defined in non-ecliptic are not being converted to ecliptic on the fly when performing fov checks.

This bug has only been made apparent after the recent #152 PR where the FOV of NEOS was switched to Equatorial.
All other FOVs are defined in Ecliptic (the best solar system frame).

Code

Specifically, all functions listed under this trait:
https://github.com/Caltech-IPAC/kete/blob/main/src/kete_core/src/fov/fov_like.rs#L15

do all calculation without checking that the FOV and arguments of their functions are in the same coordinate frame.

I do not believe this is the only place where this bug may exist however.

Discussion

The underlying issue here is that vectors on the rust side do not carry around frame information. I was aware of this as a possible source of bugs for a while (the last ~6 months), but not updated the code as it would be a breaking change for users, requiring them to update files.

This would be solved by the closed draft PR #75 , which was designed to make this bug impossible by construction.
That PR would put frame information throughout the backend with strict typing.

Unfortunately, the reason I closed that PR is that it would be a breaking change to files already saved, I may need to accept that this change is necessary and rebuild existing files.

A part of the reason for the PR #146 which added Parquet file support was so that the files would be decoupled from this or similar changes.

@dahlend dahlend added bug Something isn't working backend Issue in the rust backend labels Nov 14, 2024
@dahlend dahlend self-assigned this Nov 14, 2024
@dahlend
Copy link
Collaborator Author

dahlend commented Nov 14, 2024

Currently this bug appears to be only impacting NEOS FOVs on the python side.

@dahlend dahlend changed the title FOV State visibility checks are not performing frame conversions NEOS FOV State visibility checks are not performing frame conversions Nov 14, 2024
@dahlend
Copy link
Collaborator Author

dahlend commented Nov 14, 2024

Possible Solutions

  • Simplest (duct tape & prayers) - Change NEOS FOVs to auto convert to ecliptic after construction, hide this from the python users.
  • Best - Finish what was started in Complete rewrite of reference frames. #75, rebuild files.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend Issue in the rust backend bug Something isn't working
Projects
None yet
Development

No branches or pull requests

1 participant