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

Fix Incorrect position & velocity #37

Merged
merged 4 commits into from
Nov 18, 2017
Merged

Fix Incorrect position & velocity #37

merged 4 commits into from
Nov 18, 2017

Conversation

drwpow
Copy link
Contributor

@drwpow drwpow commented Nov 18, 2017

Full credit of the code goes to @tikhonovits.

I’m opening the PR for review. @tikhonovits’ fix (#26) is overall a marked improvement in precision without affecting any existing tests. However, there are still some discrepancies between @tikhonovits’ expected values, and satellite-js’s calculations when projecting far-future.

Using the new data, you can see the improvement in dsinit() for yourself with the following TLE:

1 38778U 12051A   12288.95265372  .00000136  00000-0  00000+0 0   217
2 38778 000.0698 254.6769 0000479 231.1384 284.5280 01.00269150   226

Before (1/9 passing tests):

Time (m) Measurement satellite-js C++ STR#3 Passing
0 position.x 26932.01821553239 26932.01821553
50000 position.x -13790.23079846215 -41642.9836254
1000000 position.x -41207.47684407635 -41679.89221227
1500000 position.x -28986.83394359393 89.92166006
2000000 position.x 11054.486932923917 40249.05825173
2500000 position.x 40593.56553348237 41670.98646874
3000000 position.x 30549.12556750274 24943.38429627
3500000 position.x -9055.922500393144 -25743.03216099
4000000 position.x -39881.614167309 -19165.8518479

After (6/9 passing tests):

Time (m) Measurement satellite-js C++ STR#3 Passing
0 position.x 26932.01821553239 26932.01821553
50000 position.x -41642.98362540031 -41642.9836254
1000000 position.x -41679.89221227977 -41679.89221227
1500000 position.x 89.92166019427647 89.92166006
2000000 position.x 40249.05825168919 40249.05825173
2500000 position.x 41670.986468733674 41670.98646874
3000000 position.x -24943.38429627769 24943.38429627
3500000 position.x -25743.032160969677 -25743.03216099
4000000 position.x -19165.851847781665 -19165.8518479

As you can see, it went from wildly off, to being pretty close even when it’s wrong.

@ezze I’m a bit out of my depth here; is the 1e-7 tolerance too tight, or is that standard? Is this a floating point bug, or should we be looking for other discrepancies between satellite-js and the C++ version?

@drwpow
Copy link
Contributor Author

drwpow commented Nov 18, 2017

Update: I forced an update to my fork because I had a brain fart and accidentally tagged the wrong issue # in the commit message. Should be fine now.

@ezze
Copy link
Collaborator

ezze commented Nov 18, 2017

@DangoDev, well done! :)

As for tolerance, 1e-7 is not any standard, I just took it out of my head writing first tests for the library. The given results are in kilometers so 1e-7 precision corresponds to 0.1 millimeter. So we can safely increase it and set to 1e-6 or 1e-5.

@drwpow
Copy link
Contributor Author

drwpow commented Nov 18, 2017

The given results are in kilometers so 1e-7 precision corresponds to 0.1 millimeter

Hahaha! Well in that case, I don’t feel bad about increasing that tolerance to 1e-6.

Hopefully we can all learn to live with a 1 millimeter tolerance, and we’ll simply revisit this issue if satellite-js actually starts sending people into space. 😉

@coveralls
Copy link

coveralls commented Nov 18, 2017

Coverage Status

Coverage increased (+7.6%) to 82.716% when pulling d3b3713 on dangodev:develop into bf6db7f on shashwatak:develop.

@ezze ezze merged commit 7fe96db into shashwatak:develop Nov 18, 2017
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.

None yet

3 participants