Bring current with 7 dec 15 vallado CPP version #35
Conversation
io.py
1. Rename afspc_mode to opsmode,
take a/i as input instead of True/False
2. Clarify which mean anomaly (no) is used,
satrec.no variable renamed to satrec.no_kozai,
internal updates to support name change
3. Make more internal variables available available to satrec
propagation.py
1. Rename afspc_mode to opsmode,
take a/i as input instead of True/False
2. Initialize whichconst variables internally once,
make values accessible in satrec
3. delete unused variables in initl
test.py
1. Replace deprecated assertRaisesRegexp with assertRaisesRegex
Notes:
- Confirmed SGP4-VER.TLE is still current
- Confirmed unit tests pass with
python -m unittest discover sgp4
|
I noticed I couldn't regenerate a TLE from the satrec.model variables - particularly, mean motion (no) was off. After going down a rabbit hole to determine the problem, I noticed David Vallado had updated the SGP4.CPP to address the issue, as well as other updates. I'm not sure if the sgp4.ext julian day routines needed updating, given the python implementation of fractions, so I've left those as-is. This update breaks two external variables: afspc_mode and model.no, but brings them current with the CPP code so update/branch with care. |
|
Thanks for noticing a discrepancy between this routine and the most up-to-date one! I'm traveling at the moment for a Python conference, but will plan to tackle this later this month when I'm done. |
|
Could you link to the source code that Vallado released? I did a quick web search but can't find the source of these changes. Thanks! |
|
Also, as noted in #19 (comment) you can use the wayback machine to find previous versions of |
|
Updated test.py to handle deprecated assertRaisesRegexp for Python 2.6/2.7 |
I note that this rolls back a5174cd which was part of trying to make the code more susceptible to Numba acceleration. The description of that commit correctly notes that it “does introduce an API change” and probably because of that warning, I have never yet released a version of this package that includes the change. If Numba is not going to prove to be our perfect answer for good performance, then I would rather stick as close to Vallado as possible, for folks who are already familiar with his version of each of these routines. So I'm inclined to accept a regression here back to the Vallado-style version of this option, and from now on to focus on (gulp!) a compiled version of the package to supply performance where people need it (for which we have other PR’s currently ongoing). @ckuethe — I'm probably going to merge this soon, which will, alas, roll back some of the attempts you made to make things Numba-friendly. Please comment here if you think that’s a wrong direction for me to take; I’m open to considering other options. But at the moment my inclination would be to merge this by sometime next week. |
|
I don't really care about the API too much, I just want it to go fast. |
|
@brandon-rhodes I've been using this fork, along with the cython accelerations from PR #38 for the back-end of https://trusat.org/ I also have a working cthonized/tsince-vectorized fork at https://github.com/interplanetarychris/python-sgp4/tree/7-dec-15-vallado-tsince-vectorize but sadly it does not accelerate performance much for my application of ~10s of points for a single satellite. I will be putting out a https://gitcoin.co/ or similar bounty in hopes of incentivizing someone in the world to get the functions working for a 10-100x speed bump per my and @ckuethe's need for speed. Depending on the results of that, it may make sense to maintain a separate "performance" fork of python-sgp4, or depending on the changes, roll them into the main repo. We shall see... I will post info about that bounty task here when I have it online - it will include a fork of the repo along with test/performance metric code to help a positive outcome. |
How does its behavior compare with PyEphem (if you have time to check, of course)? Several folks have been happy with PyEphem through the years, but I can't imagine why it would be any faster than the same routine in Cython. |
| @@ -139,17 +139,18 @@ def twoline2rv(longstr1, longstr2, whichconst, afspc_mode=False): | |||
| line[63] == ' '): | |||
|
|
|||
| _saved_satnum = satrec.satnum = int(line[2:7]) | |||
| # classification = line[7] or 'U' | |||
| # intldesg = line[9:17] | |||
| satrec.line1 = line | |||
There was a problem hiding this comment.
Was this debugging code, or an idea for a new feature? Vallado's routines don't save the lines, and here in Python there may be a danger: will users do things like take a satellite, adjust a parameter, and expect .line1 to now reflect the new parameter? It might be safer to not cache this data since the source library does not.
There was a problem hiding this comment.
I've finally had time to finish reading these changes and compare them to the changes in the C++ code. Could you take a look at these two quick questions I have (and the one I already made outside the review, because I pushed the wrong button)? Once they're resolved, I look forward to merging this!
| two_digit_year = int(line[18:20]) | ||
| satrec.epochdays = float(line[20:32]) | ||
| satrec.ndot = float(line[33:43]) | ||
| satrec.nddot = float(line[44] + '.' + line[45:50]) | ||
| nexp = int(line[50:52]) | ||
| satrec.bstar = float(line[53] + '.' + line[54:59]) | ||
| ibexp = int(line[59:61]) | ||
| # numb = int(line[62]) | ||
| # elnum = int(line[64:68]) | ||
| satrec.ephtype = line[62] |
There was a problem hiding this comment.
If I'm reading Vallado's code correctly, it takes satrec.ephtype and satrec.elnum from line2 instead of line1. Can we switch to do the same, for compatibility in case the lines disagree?
| satrec.inclo = float(line[8:16]) | ||
| satrec.nodeo = float(line[17:25]) | ||
| satrec.ecco = float('0.' + line[26:33].replace(' ', '0')) | ||
| satrec.argpo = float(line[34:42]) | ||
| satrec.mo = float(line[43:51]) | ||
| satrec.no = float(line[52:63]) | ||
| #revnum = line[63:68] | ||
| satrec.no_kozai = float(line[52:63]) |
There was a problem hiding this comment.
Well, drat. This creates an incompatible change that will crash Python programs that looked at the .no attribute previously. After merging this, I might add a .no property that returns this value, to prevent breakage. But I agree that we should follow Vallado, as a higher priority.
|
It happens that I have some time today to spend on the library, so I'll make the suggested changes myself post-merge. Thanks again for noticing that the algorithm was out of date! |
|
Oh, and, in case other followers of this thread have not seen it, there now seems to be a fast compiled vectorized SGP4 for Python: https://pypi.org/project/cysgp4/ I am going to experiment with using its results in Skyfield, in which case I'll be happy to recommend its use in place of this pure-Python |
|
And I've opened an issue asking some questions about that library, if anyone wants to follow along: |
io.py
take a/i as input instead of True/False
satrec.no variable renamed to satrec.no_kozai,
internal updates to support name change
propagation.py
take a/i as input instead of True/False
make values accessible in satrec
test.py
Notes:
python -m unittest discover sgp4