added whichconst to twoline2rv

[zouhairm]

Alright, this hopefully addresses #47 by making the new API expose the gravity constant while still providing a convenient way to call directly to a lower level API if speed is of the essence.

Based on your comments, here is what I have done:

• I turned the twoline2rv function into a 'protected' function _twoline2rv which takes an extra positional argument to specify the gravity constant. This argument is expected to be an integer representation of the gravconstant enum

• I created a new twoline2rv function (implemented in python) which wraps _twoline2rv and handles parsing the conditional keyword argument whichconst. This one can be either a string, an earth_gravity constant object, or an int/enum.

Since performance is of importance, I used %timeit to get an idea of the impact, results of %timeit satellite = sgp4.api.Satrec.twoline2rv(s, t) are as follow:

• Basecase with sgp4 v2.3: 2.97 µs ± 32.8 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)

• With new api and calling the protected _twoline2rv directly: 3.09 µs ± 45.3 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)

• With new api and calling the 'user friendly' twoline2rv: 3.31 µs ± 45.9 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)

So about 10% slower with the user friendly version which makes the API fully backward compatible, and for those who can't afford a much slower initialization calling _twoline2rv directly is only 3% slower. Hopefully this is acceptable :)

Let me know if you have comments or suggestions.

P.S. I tried also with using PyArg_ParseTupleAndKeywords directly in wrapper.cpp, but it's easier to deal with the different types on the python side, and I think PyArg_ParseTupleAndKeywords is likely slower than parsing tuples directly.

Also, here is some code for testing

import sgp4
import sgp4.api
import sgp4.io
from sgp4.earth_gravity import wgs72, wgs84

import datetime as dt
import numpy as np
import itertools

s = '1 25544U 98067A   19343.69339541  .00001764  00000-0  38792-4 0  9991'
t = '2 25544  51.6439 211.2001 0007417  17.6667  85.6398 15.50103472202482'
%timeit satellite = sgp4.api.Satrec.twoline2rv(s, t)

TLE_LINES = (
    "1 43204U 18015K   18339.11168986  .00000941  00000-0  42148-4 0  9999",
    "2 43204  97.3719 104.7825 0016180 271.1347 174.4597 15.23621941 46156",
)

REF_DATE = dt.datetime(2019, 1, 1)

date_args = (REF_DATE.year,
        REF_DATE.month,
        REF_DATE.day,
        REF_DATE.hour,
        REF_DATE.minute,
        REF_DATE.second + REF_DATE.microsecond * 1e-6)

date_jday = sgp4.api.jday(*date_args)

io_wgs72 = np.array(sgp4.io.twoline2rv(TLE_LINES[0], TLE_LINES[1], wgs72).propagate(*date_args)).flatten()
io_wgs84 = np.array(sgp4.io.twoline2rv(TLE_LINES[0], TLE_LINES[1], wgs84).propagate(*date_args)).flatten()

api_wgs72 = np.array(sgp4.api.Satrec.twoline2rv(TLE_LINES[0], TLE_LINES[1], wgs72).sgp4(*date_jday)[1:]).flatten()
api_wgs84 = np.array(sgp4.api.Satrec.twoline2rv(TLE_LINES[0], TLE_LINES[1], wgs84).sgp4(*date_jday)[1:]).flatten()

for (a, u), (b, v) in itertools.combinations([
        ('io_wgs72 ', io_wgs72), ('api_wgs72', api_wgs72), 
        ('io_wgs84 ', io_wgs84), ('api_wgs84', api_wgs84)], 2):

    err = u-v
    p_err = np.linalg.norm(err[0:3])
    v_err = np.linalg.norm(err[3: ])*1e3
    print(f'Diff between {a} & {b}: pos = {p_err:.2f} km , vel = {v_err:.2f} m/s')

Output:

Diff between io_wgs72  & api_wgs72: pos = 0.00 km , vel = 0.00 m/s
Diff between io_wgs72  & io_wgs84 : pos = 0.88 km , vel = 0.98 m/s
Diff between io_wgs72  & api_wgs84: pos = 0.88 km , vel = 0.98 m/s
Diff between api_wgs72 & io_wgs84 : pos = 0.88 km , vel = 0.98 m/s
Diff between api_wgs72 & api_wgs84: pos = 0.88 km , vel = 0.98 m/s
Diff between io_wgs84  & api_wgs84: pos = 0.00 km , vel = 0.00 m/s

[zouhairm]

I think having an integer as the argument to twoline2rv compromises ease of use and readibility of the API.

What I suggest is the following:

• keep the version that takes a required enum integer as api._twoline2rv
• make api.twoline2rv take an optional whichconst which is either a string or an enum integer, but not an earth_gravity object (thus not having to include the earth_gravity module in api)
• make compat.twoline2rv take an optional whichconst, which can also be an earth_gravity object to provide backward compatibility with versions < 2.0. Although I'd argue this is not really needed since io.twoline2rv still exists. The goal of this PR is really to provide a way to specify gravity constant for the new api, which is achieved by the previous 2 points.

Let me know if that's a reasonable compromise. It makes the new api.twoline2rv user friendly while still avoiding dependency on earth_gravity.py

[brandon-rhodes]

To start with your third point:

… Although I'd argue this is not really needed since io.twoline2rv still exists.

Happily, I agree with you! Your inclusion of the old-style gravity constants in your first draft had made me wonder if you thought that the change from wgs84 → WGS84 was going to be too onerous for folks wanting to upgrade from the old to the new API. If instead you think that it’s fine for folks to stay with the earlier API io.twoline2rv(…, wgs84) until such time as they can switch to api.twoline2rv(…, WGS84), then I am very happy to avoid the additional complexity of a new module. Thanks for clarifying instead of just implementing it when I first mentioned it!

Your first two points, then, are motivated by:

I think having an integer as the argument to twoline2rv compromises ease of use and readability of the API.

Could you show an example of the change in use, and an example of the change in readability, and comment on the loss? I myself was comparing these in my imagination:

sat = twoline2rv(…, wgs84)    # old
sat = twoline2rv(…, WGS84)    # new

The only difference I can see is that the new convention actually looks like the real-world all-caps spelling of the abbreviation for "World Geodetic System", whereas the old way puts it in lowercase. Letting people switch to the string 'wgs84' would (a) seem to cost an extra function + hash table lookup + string comparison, (b) turns the signature of that argument from "integer" to "string OR integer" which my static-types friends would remind me creates something of a mess compared to having a single type expected, and (c) means the user gets no immediate error if they misspell. Any string will look good to pyflakes or their type checker; whereas misspelling WGS84 will be immediately caught as they type because it would be an undefined symbol.

Could you outline the example lines that you were thinking of, and how the string-based convention in that case helps the user with their code?

[zouhairm]

I was trying to avoid the case of having to call: twoline2rv(line1, line2, 1)

But I think if we expose the enums as named variables from the api module (currently they are inside of vallado_cpp ) then I agree with sticking with the keyword argument being an integer instead of a string given the other points.

I'll make the changes and update the PR.

[brandon-rhodes]

But I think if we expose the enums as named variables from the api module (currently they are inside of vallado_cpp ) then I agree with sticking with the keyword argument being an integer instead of a string given the other points.

Ah, excellent point! Yes, please import all three constants into api. I had entirely failed to notice that the PR didn't do that yet. Thanks!

[zouhairm]

working on adding tests now, but wanted to push to make sure this is in the right direction.

• got rid of twoline2rv and api.twoline2rv now takes line1, line2 and whichconst as an optional enum. parsing is all done in C++. As far as I can tell, this is pretty much the same performance as before. Note that that no error checking is thrown if the enum is not valid, but results of propagator are nan (i.e. 'silent failure' - more on this below)
• exposed enums in api - in process realized that should probably take care of the non-accelerated version
  • model.twoline2rv, the non accelerate version, also accepts an enum now. note that unfortunately I had to hardcode the values in there for WGS72, etc. because I assumed that there is no access to the header file. As long as the user is using the enums and not literal integers, then there should be no issue. If they are using literal integers, in the case that the enums have different values than those hardcoded, different behavior can ensue ...
  • speaking of different behavior, model.twoline2rv will throw an exception if the enum value is not valid. if you want exactly the same behavior, lmk and I can make wrapper.twoline2rv do the same (at the cost of one additional check)

[brandon-rhodes]

Thanks! I'll plan to do a new release this month to more widely share the improvement you've made.

[zouhairm]

Thanks @brandon-rhodes - any chance we can do a minor release sooner as I'm relying on this in a different project I need to release (and referring to a branch/commit vs. a proper version number is not ideal)

[brandon-rhodes]

I'll see if I can have things in shape by the end of the weekend for a Monday release.