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

Added function json2satrec in io.js #122

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from

Conversation

spacenewb
Copy link

@spacenewb spacenewb commented Aug 15, 2023

Added function json2satrec in io.js to initialise sgp4init object and build satrec object from OMM data in JSON format. Ref: A New Way to Obtain GP Data (aka TLEs).

This is the new way of distributing TLEs and would become the new standard soon. It is better to port our functions slowly for compatibility.

#105

Initialise `sgp4init` object and build `satrec` object from OMM data in JSON format. Ref: https://celestrak.org/NORAD/documentation/gp-data-formats.php
@thkruz
Copy link
Collaborator

thkruz commented Aug 16, 2023

Thanks for taking the time to help future proof this library!

We need to add some unit tests for this new function. Might be smart to have at least a few that make sure it is getting the same answer as the original function.

I'll take a look in the airport tomorrow, but the comment block on this looks incorrect. It mentions more input functions than are available. Additionally there are a lot of leftover comments from copying the original function - if we are modernizing the library there is no benefit of keeping old comments that don't apply.

Copy link
Collaborator

@thkruz thkruz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall I think this is in the right direction. It needs a little polish. I will try to find some time to write tests in the near future, but if anyone else can it would be great. We need to validate that it makes the same satrec object as the main function.

src/io.js Outdated Show resolved Hide resolved
src/io.js Outdated Show resolved Hide resolved
src/io.js Outdated Show resolved Hide resolved
src/io.js Outdated
*
* Provide the OMM JSON format data as `jsonobj`,
* and select which standard set of gravitational constants you want
* by providing `gravity_constants`:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unclear if you can actually provide gravity constants or if this old comments too.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this ported code, I cannot see the use of the gravity constants in any of the scorce code methods. Maybe this can be a new issue to be implemented.

src/io.js Outdated Show resolved Hide resolved
src/io.js Outdated Show resolved Hide resolved
@@ -6,7 +6,7 @@ import sgp4init from './propagation/sgp4init';

/* -----------------------------------------------------------------------------
*
* function twoline2rv
* function twoline2satrec
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain why you are updating the name? Did you update the comments above too?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because classically, rv stands for satellite state vector: [rx, ry, rz, vx, vy, vz]. This represents the position and velocity in the x,y and z directions. But this function actually returns a satrec object.

This is not the same as a satellite state vector. A satrec object is a dictionary containing all the sgp4 satellite information [sgp4io.cpp].

I have not modified any other comments above. I figured that the old way on naming the function was not in-line with its modern interpretation of the term rv and satrec.

@spacenewb
Copy link
Author

Overall I think this is in the right direction. It needs a little polish. I will try to find some time to write tests in the near future, but if anyone else can it would be great. We need to validate that it makes the same satrec object as the main function.

I did perform some basic tests, but I am not so proficient in JS, to write unit tests. But there seems to be a way to compare two objects using a method named _.isEqual from a library called Lodash. Everything seemed to be exactly the same except for the difference in the satrec.epoch.

The old and the new methods seemed to vary in the 8th decimal place due to some floating point magic which was beyond my understanding as a non-CS background person. this translates to a difference in a couple of milliseconds.

If there are ways to check if these are within a certain tolerance while writing the tests, that would be optimal.

Copy link
Collaborator

@thkruz thkruz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two minor corrections and then the only thing left to do is the jest testing. I am going to write them right now as a PR into your develop branch (as soon as I figure out how to deconflict yours with shashwatak's).

src/io.js Outdated
* vallado, crawford, hujsak, kelso 2006
--------------------------------------------------------------------------- */
function json2satrec(jsonobj, opsmode='i') {
const opsmode = 'i';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

opsmode is passed as a parameter and defaulted to 'i'. This line of code would create a separate variable and cause the input parameter to be ignored. Please delete this line.

src/io.js Outdated

satrec.satnum = jsonobj.NORAD_CAT_ID.toString();

var epoch = new Date(jsonobj.EPOCH + 'Z');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The use of var should be replaced with const.

thkruz added 3 commits August 19, 2023 15:50
io.js now has two exports and the import statements in a few files needed to be updated
Fixed a bug, reduced the number of lines of code, and converted one calculation into a constant for performance
@thkruz
Copy link
Collaborator

thkruz commented Aug 19, 2023

I opened a PR (spacenewb#1) on your PR to incorporate the changes I suggested and add the testing.

Copy link
Collaborator

@thkruz thkruz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ezze this looks good to me at this point. It provides a valid method for using OMM format without causing any degradation to the current baseline.

Implemented function `sunPos`, to calculate sun position. This resolves issue  shashwatak#68 .

The name of the function is slightly changed for clarity and added references.

Cleaned the ported code `sun.js.txt` as provided in the original issue.

Removed `var` declarations within the function to augment performance.
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.

2 participants