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

Fixed ECIToLookAngles #4

Merged
merged 1 commit into from
May 8, 2019
Merged

Conversation

ainagarciaes
Copy link
Contributor

ECIToLookAngles was incorrectly implemented based on the cited reference on top of the function. I implemented it again, and checked the output with libpredict-based softwares to assess its functionality.

@joshuaferrara
Copy link
Owner

This is interesting, thanks for bringing it up.

I believe the reason behind between the difference of the initial implementation and what's on Celestrak was to try and reuse some other conversion methods to trim down what was occurring in ECIToLookAngles, though it looks like that introduced some inaccuracies.

Can you provide the output/code you use to verify with libpredict? I'd like to try and use it to verify some other parts of the library, in addition to having a source to reference back to if someone has future issues.

Additionally, it would be wise to have test cases for the conversions in addition to the test cases for the SGP/SDP algorithms. This is something I'd like to add in the future so that inaccuracies like these can be discovered before they exist.

Thank you again for the PR.

@joshuaferrara joshuaferrara merged commit c578fe8 into joshuaferrara:master May 8, 2019
@joshuaferrara
Copy link
Owner

Alright, sorry for the late response - had to find some time to review this once more and it looks good to me.

Thanks again!

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

Successfully merging this pull request may close these issues.

None yet

2 participants