-
Notifications
You must be signed in to change notification settings - Fork 145
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
Sgp4 validation #108
Sgp4 validation #108
Conversation
@thkruz What a huge work you did, mate! Awesome!!! Could you please describe a little bit in details how you generated snapshot data for tests. I mean: where did you take TLE inputs and calculation outputs corresponding to them? I see it's based on original C++ library but I feel that we must provide more explainations for next generations so they could recreate these data if they need. :) |
@ezze something like this? How I Tested Against Sgp4Prop
The probability of me parsing the sgp4prop output incorrectly and then calculating the same wrong value with satellite.js to 5 decimal places seemed low enough to ignore that possibility. I could have tested more data points for each satellite but getting jest kept trying to do too much at once and ran out of memory. There is probably a way to get it to do the tests in order and garbage collect as needed but I couldn't figure it out. 6 data points felt sufficient. I chose the python wrapper so that I didn't need to compile anything. I would recommend the same unless you have an environment already setup for the other languages included in sgp4prop. How I Made the SnapshotsNow that I knew satellite,js got the right answer at those 6 times I created 52 more tests (test maker included in the PR) that tests if satellite.js still gets the same snapshot as it did the last time it ran. On the initial run Jest will generate a snapshot to compare against in the future - this is a built in feature - that initial snapshot is what I uploaded. So by propagating the same TLE to the same 6 points in time that passed my sgp4prop test I know that my snapshots are accurate without uploading the actual values sgp4prop got. If the math gets altered, the snapshots will no longer match and the test will fail. |
@thkruz Thanks, that's what I was asking for! Let's try to optimize these 52 files with tests by "merging" them into a single file, and then, I guess, we are good to go. Probably, it would be better to include it in a major release. @TSKelso @davidcalhoun @tikhonovits @DangoDev Do you also want to review it? |
@ezze I had split them on purpose so that Jest would run them in parallel, but will defer to your expertise on the best way to organize stuff. (Hard to believe it was only a few years ago that you informed me that I should get npm and catch up to the rest of the world). Let me know if you want to do the merging or if you want me to. I will be away all of September, so I might be a bit slow. |
Good stuff! I agree that this is a significant change that warrants a major release! I didn't realize Jest needed tests in separate files for parallelization but I confirmed it, makes sense to me! Many thanks for documenting the testing development. |
@thkruz Ok, if you want to split them for the sake of parallel run then at least you can create a single utility function including the repeating code. And each test will look something like: sgp4-catalog-<n>.test.js import { sgp4test } from './sgp4test';
sgp4test(<n>); where Moreover, these little test files could be generated easily by some Node script so you don't have to write them by yourself. |
@thkruz Let's look when you optimize test scripts, and then I can merge it. If you can't due to lack of time, let me know. I will do it by myself then in upcoming days. All the most difficult stuff is already done by you, mate, so all the rest is super easy. ;) |
Fixed in 5.0.0. @vinifraga Let me know whether it works for you. |
This fixes the accuracy issues raised in #107 and a second issue with satrec initialization.
Screenshot showing the sgp4prop tests running when the snapshots were made.