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

incorrect position & velocity #26

Closed
weatherlym15 opened this issue Jan 25, 2017 · 25 comments
Closed

incorrect position & velocity #26

weatherlym15 opened this issue Jan 25, 2017 · 25 comments
Labels

Comments

@weatherlym15
Copy link

I followed what you have in your README.md where I made timeSinceTleEpochMinutes = increments of 300.

var tleLine1 = '1 25544U 98067A 13149.87225694 .00009369 00000-0 16828-3 0 9031',
tleLine2 = '2 25544 051.6485 199.1576 0010128 012.7275 352.5669 15.50581403831869';
var satrec = satellite.twoline2satrec(tleLine1, tleLine2);
var positionAndVelocity = satellite.sgp4(satrec, timeSinceTleEpochMinutes);

But when I print out the x, y, and z positions and velocity values, the values differ from what I get using the python library you said you copied your satellite-js library from. At first the differences are small but they become a problem as my 300 second increments get larger and larger

Am I missing a step?

@tikhonovits
Copy link

Dear @shashwatak,

unfortunately, I have to confirm the trend as described by @weatherlym15.

Indeed, for time instances close to the TLE epoch, the results provided by the js version match to the corresponding ones by the c++ version. Note that my js results are in agreement with those provided for benchmarking purposes, but the time instances there are quite close to the TLE epoch, and hence, the problem is not revealed. However, for time differences wrt TLE epoch larger than a few months, it becomes evident that the results between the two versions are starting to deviate.

Below, as an example you may find results provided by both versions for the Astra 2F (38778) satellite using 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

By comparing the two versions, it becomes clear that the results are in agreement only for the TLE epoch time instance. Note that I have verified the validity of my c++ results by equivalent results using the STK software for the TEMEofEpoch system of coordinates.

After enough trials and comparisons, I have concluded that perhaps either the js version has a bug, or progressively loose numerical precision, or the Cartesian position/velocity results versus time since TLE epoch are not in the True Equator, Mean Equinox (TEME) coordinate system, as the results provided by the c++ are.

Please help me to resolve this matter, since your js version is quite useful to all of us.

Thanks for your time,
Nikos


*** Results Obtained for Astra 2F (38778) Based on C++ Version ***


     Time (min)         x (km)           y (km)            z (km)       Vx (km/s)    Vy (km/s)    Vz (km/s)   Y    M D     Time
       0.00000000   26932.01821553   32442.36380581      18.67203987 -2.365855637  1.963833427 -0.004652773
  500000.00000000  -41642.98362540    6521.85388710    -484.73552503 -0.475966361 -3.038100957  0.005837330  2013  9 27  4:11:49.281394
 1000000.00000000  -41679.89221227   -6406.44765047    -888.19222905  0.466036434 -3.038415548  0.041394568  2014  9  9  9:31:49.281416
 1500000.00000000      89.92166006  -42138.48887821     783.16731078  3.073618177  0.008372197  0.102829394  2015  8 22 14:51:49.281398
 2000000.00000000   40249.05825173  -12456.01761882    2073.91657116  0.910791707  2.935583446 -0.045177132  2016  8  3 20:11:49.281421
 2500000.00000000   41670.98646874    6091.87422335    1846.43614861 -0.438088814  3.040224736 -0.144569657  2017  7 17  1:31:49.281403
 3000000.00000000  -24943.38429627   33846.85571434   -3202.53618091 -2.473012005 -1.826475386 -0.041201988  2018  6 29  6:51:49.281425
 3500000.00000000  -25743.03216099   33183.50696319   -3784.96326117 -2.428125057 -1.885980854 -0.019143710  2019  6 11 12:11:49.281407
 4000000.00000000  -19165.85184790  -37493.19360463    1884.67124427  2.732242938 -1.382445535  0.284603946  2020  5 23 17:31:49.281430

*** Results Obtained for Astra 2F (38778) Based on Javascript Version ***


Time since epoch: 0 min 
Position (km): (x = 26932.01821553236, y = 32442.363805806563, z = 18.672039872509902)
Velocity (km/s): (Vx = -2.3658556373925705, Vy = 1.9638334268968463, Vz = -0.004652773306378888)

Time since epoch: 500000 min
Position (km): (x = -13790.23079846215, y = 39843.11842649649, z = -298.26415348078467)
Velocity (km/s): (Vx = -2.9054892440023568, Vy = -1.0058968645002497, Vz = -0.02848165004581589)

Time since epoch: 1000000 min
Position (km): (x = -41207.47684407635, y = 8867.498679969154, z = -1031.9048243201808)
Velocity (km/s): (Vx = -0.6472942239720777, Vy = -3.005855532153245, Vz = 0.015608178622012314)

Time since epoch: 1500000 min
Position (km): (x = -28986.83394359393, y = -30618.058288413285, z = -404.3298355701939)
Velocity (km/s): (Vx = 2.2304434637733364, Vy = -2.1132827091217172, Vz = 0.11385995501982366)

Time since epoch: 2000000 min
Position (km): (x = 11054.486932923917, y = -40658.27635624185, z = 1635.5945933881073)
Velocity (km/s): (Vx = 2.964198038268495, Vy = 0.8099990524788586, Vz = 0.1032917723914456)

Time since epoch: 2500000 min
Position (km): (40593.56553348237, y = -11134.784332207368, z = 2486.8607168434683)
Velocity (km/s): (Vx = 0.8175136377497744, Vy = 2.96286742092495, Vz = -0.07841253986503455)

Time since epoch: 3000000 min
Position (km): (x = 30549.12556750274, y = 29063.493629407978, z = 162.3154320499707)
Velocity (km/s): (Vx = -2.1123431811282027, Vy = 2.221533024821393, Vz = -0.23683632574840144)

Time since epoch: 3500000 min
Position (km): (x = -9055.922500393144, y = 41047.65542346771, z = -3310.073213239884)
Velocity (km/s): (Vx = -2.9972566649957533, Vy = -0.672256949943488, Vz = -0.1351650134962626)

Time since epoch: 4000000 min
Position (km): (x = -39881.614167309, y = 13207.092316204795, z = -3580.7279528653353)
Velocity (km/s): (Vx = -0.9793148013533146, Vy = -2.909201062829499, Vz = 0.17800703001792312)

@shashwatak
Copy link
Owner

@weatherlym15 @tikhonovits thank you for your patience, I have been very busy with work and travel. If you guys don't mind, ping me on Wednesday (march 13), I should be able to set aside some time to investigate this issue.

@tikhonovits
Copy link

Dear @shashwatak,

as you see I was a little bit unpatient...!

Fortunately, I found the source of the progressive deviation between js and c++ versions.

By comparing all members of satrec between the two versions, I found the following values as different wrt to the js version:

satrec.del1	-6.3969223360555608e-13	double
satrec.del2	1.4103037029390144e-11	double
satrec.del3	1.9783041987769300e-12	double
satrec.irez	1	int
satrec.xfact	-0.0043750247186120847	double
satrec.xlamo	0.75935747679288923	double
satrec.xli	0.76396171705721150	double
satrec.xni	0.0043746456895873351	double

The property that makes the difference of course is irez, which for some reason in the js version is equal to zero. After going one step further, I realized that the two ifs in "deep space initialization" section within the dsinit function (lines 2507-2515) do not work as expected. Hence, by substituting these if conditions by the way they are written in the c++ version, i.e.,

//  -------------------- deep space initialization ------------
        irez    = 0;
     if ((nm < 0.0052359877) && (nm > 0.0034906585))
         irez = 1;
     if ((nm >= 8.26e-3) && (nm <= 9.24e-3) && (em >= 0.5))
         irez = 2;

everything worked fine! All position and velocity results between js and c++ versions now perfectly match.

I suggest to further substitute all if conditions, which now are written as the problematic ones, with the original ones in the c++ version to be sure that all conditions will be working properly under different settings.

Hope that helped.

Best,
Nikos

@shashwatak
Copy link
Owner

shashwatak commented Mar 14, 2017

@tikhonovits If you would be so kind as to make a PR with the necessary fixes, I will gladly accept it :) Please make sure the tests in the tests directory still pass. Also, please add your name to the contributors list end of the Introduction in the README.md :)

@ezze
Copy link
Collaborator

ezze commented Apr 14, 2017

I also found this issue last night migrating from RequireJS to CommonJS. ESLint pointed to these lines and I left them as is because wasn't sure how to fix them properly.

@shashwatak, if you feel that code refactoring proposed by pull request mentioned above is acceptable, I can create another pull request later to fix this issue (like @tikhonovits proposed) and add some test cases based on Astra 2F results from C++ version.

@drwpow
Copy link
Contributor

drwpow commented Nov 13, 2017

@ezze @tikhonovits are there any open PRs to address this? I’d be happy to open one myself but don’t want to take credit for anyone else’s work.

@ezze
Copy link
Collaborator

ezze commented Nov 13, 2017

@DangoDev, I decided to not make any changes to this repo until this big pull request is merged or rejected. Seems that @shashwatak is not supporting the repo anymore.

@drwpow
Copy link
Contributor

drwpow commented Nov 13, 2017

@ezze That makes sense. I looked through that and saw you hadn’t sneakily changed the values for src/dsinit.js.

Is it time to discuss forking this repo?

@ezze
Copy link
Collaborator

ezze commented Nov 13, 2017

@DangoDev +1 for forking. But probably a person having a good knowledge in orbital mechanics is required to support it further.

@drwpow
Copy link
Contributor

drwpow commented Nov 13, 2017

But probably a person having a good knowledge in orbital mechanics is required to support it further.

hides under desk

@tikhonovits
Copy link

@DangoDev @ezze If any help will be needed with orbital mechanics, I will be happy to contribute. At least the bug mentioned in my previous post must be corrected.

@shashwatak
Copy link
Owner

shashwatak commented Nov 14, 2017 via email

@drwpow
Copy link
Contributor

drwpow commented Nov 14, 2017

@shashwatak That’s good news! Glad to hear it.

And no need to apologize! You’ve done some incredible work on this project that you should be very proud of. Nothing to be sorry for. But all in all, glad to hear your good work can continue moving forward.

@davidcalhoun
Copy link
Contributor

@shashwatak No worries, we all have lives! Appreciate you giving @ezze collaborator rights to keep this awesome project going. :)

@ezze
Copy link
Collaborator

ezze commented Nov 14, 2017

Hey, @shashwatak! Great to hear you again! :) Thank you so much for your trust, I accepted your inventation. Hope that we will be able to move this project forward.

  1. @shashwatak, one very important thing is to gain an access to managing of NPM package, otherwise I have no chances to publish new versions of the library to NPM. As I can see, npm owner command is meant for that. Currenly, only you is able to manage the package:

    $ npm owner ls satellite.js
    shashwatak <shashwatak@gmail.com>
    

    I guess that you should run something like this to grant me the access:

    $ npm owner add ezze satellite.js
    
  2. I also want to have Travis CI support here but, unfortunatelly, I can't enable it as collaborator. @shashwatak, could you, please, enable it for this repository?

  3. In order to not pollute this issue with common talks on this repo, what do you think, guys, about moving further discussions to, say, Gitter? @shashwatak, can you enable a chat for this repository there?

  4. Probably, we will want to have a coverage for tests in the future, fortunatelly, I had a luck to enable Coveralls by myself.

  5. First of all, I plan to merge my recent pull request related to Common.js support already mentioned above and update dependency libraries. I hope that you all, guys, @DangoDev @tikhonovits @davidcalhoun give it a try and tell me if there is something wrong with it. I'll try to do it today.

  6. I also think of creating new branches, at least a dev branch to have some git workflow here. Making new dev branch as default we will force collaborators contributors cloning the repo to rebase their feature branches to this dev branch and leave master branch clean for releases only.

  7. After all, @DangoDev or any other, can fix this issue and make a pull request. :)

@ezze ezze added the bug label Nov 14, 2017
@ezze
Copy link
Collaborator

ezze commented Nov 14, 2017

5th and 6th of the previous comment are done. @DangoDev, you can give a try to fix this issue but before that let's discuss a Git workflow to use (I propose to branch your pull request from develop branch). See #35.

@drwpow
Copy link
Contributor

drwpow commented Nov 14, 2017

@ezze all that sounds good to me. I’ll give your CJS branch a poke later this week. As for the workflow, that all works for me. I’ve commonly seen it both ways—either master is the stable version, or master is the latest (unstable) version. I don’t have an opinion one way or the other; we can go with what you proposed:

  • master is current stable version
    develop is the latest, in-progress development version

Any PRs are forked from develop and re-merged there.

I’m also open to using Gitter. I’ve never tried it before, but I’ll give it a shot.

@drwpow
Copy link
Contributor

drwpow commented Nov 14, 2017

But back to this issue, I can open up a PR this week and try to implement @tikhonovits’ patch unless someone else beats me to it. I’ll compare it to the target values and hopefully we’ll be able to hit those with just this one patch.

@ezze
Copy link
Collaborator

ezze commented Nov 14, 2017

@DangoDev, please write some tests relying on input data and results provided by @tikhonovits in your pull request. Probably adding these values to existing test/sgp4.json file will be enough.

As for workflow, let's wait for what @shashwatak says. If he leaves master branch as the default one then it will remain a development branch. The most important things now are to gain access to NPM package and to enable Travis CI.

@shashwatak
Copy link
Owner

TravisCI support is now enabled, i believe
Gitter should be up as well
WIP to grant ownership of npm package to @ezze

@shashwatak
Copy link
Owner

@ezze you now have npm ownership :)

@shashwatak
Copy link
Owner

I also went ahead and protected both master and develop from deletion and force push, including from myself. I have a bad tendency to amend commits and force push sometimes 😬 this should put an end to that :)

@ezze
Copy link
Collaborator

ezze commented Nov 17, 2017

@shashwatak, Travis CI is configured for the repo. It lints, tests and builds the code for each new commit in master and develop branches.

Gitter chat is available, everyone interested can join it now (the link is added to README.md).

I also see myself in contributors' list of the npm package.

@DangoDev, start your pull request to fix this issue branching from develop.

@ezze
Copy link
Collaborator

ezze commented Nov 18, 2017

Fixed by #37.

@ezze
Copy link
Collaborator

ezze commented Dec 22, 2017

The fix is finally released in 2.0.0. You can try to update the library and give it a try.

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

No branches or pull requests

6 participants