Re: [Orekit Developers] unnecessary "for" loops in "org.orekit.propagation.analytical.tle" ?

First let apologies for the (lack of) formatting of my original posting!

To Pablo: I've explored the C++ Vallado code in great detail and felt that it was somewhat opaque. It's FORTAN origins (and conventional late-1950s usage) have resulted in variable names are that hard to interpret. I also found that the scope of variables was uncontrolled .. a temporary variable in use for a only a few lines has the same scope as variables active throughout a whole major function. Swift allows Unicode characters in variable names and I'm experiments with changing variable names to use those (cosi₀ and theta² for examples). In my Objective-C version I've localized variable scopes also. I'll look at your Scala with much interest.

To Luc: I will open a bug in the forge.

Thanks for your thoughts .. Gavin

On Wed, May 31, 2017 at 5:50 AM, MAISONOBE Luc <luc.maisonobe@c-s.fr> wrote:

Gavin Eadie <gavin+orekit@umich.edu> a écrit :

Hi Gavin,

The package "org.orekit.propagation.analytical.tle" is a Java implementation
of the algorithms described in the paper "Revisiting Spacetrack Report #3
(AIAA 2006-6753)." I am using the Orekit implementation as a partial basis
for a re-coding of those algorithms in Swift.

That requires me to look closely at conversions involving language constructs
that are available in Java but not in Swift. One of these is the "for
( ; ; )" control statement, and I've tripped over a usage in Orekit that seems
odd .. it’s computationally correct, but unnecessarily complicated. In the
file DeepSDP4.java, a constant is defined:

private static final int SECULAR_INTEGRATION_ORDER = 2;

and later in that same file, two loops of the following form:

for (int j = 0; j < SECULAR_INTEGRATION_ORDER; j += 2) {
derivs[j] = term1a + term2a;
derivs[j + 1] = term1b + term2b;
if ((j + 2) < SECULAR_INTEGRATION_ORDER) {
term1a = -term1a;
term2a *= -4.0;
term1b = -term1b;
term2b *= -4.0;
}
}

as I read this, the "for" loop will only execute once (with j = 0), and the
contained "if" condition will never be true. That reduces the above code to:

derivs[0] = term1a + term2a;
derivs[1] = term1b + term2b;

to check this, I made this (and one other similar near-by) change and re-
ran the unit tests with no failure.

There is one more "for" loop that will execute only once (with j = 2):

for (int j = 2; j <= SECULAR_INTEGRATION_ORDER; ++j) {
xlpow *= xldot;
derivs[j - 1] *= xlpow;
delt_factor *= delt / (double) j;
xli += delt_factor * derivs[j - 2];
xni += delt_factor * derivs[j - 1];
}

Am I being incredibly stupid and missing something obvious?

You are not stupid and your analysis is correct. Indeed, test coverage confirms
the inner loop is never run (see <https://www.orekit.org/jenkins/job/Orekit/395/jacoco/org.orekit.propagation.analytical.tle/DeepSDP4/>).

The structure of the code was heavily based on the C++ version by David A. Vallado,
Paul Crawford, Richard Hujsak, T.S. Kelso at celestrak. Some changes were introduced
to first adapt the API to Orekit propator and also to avoid recomputing over and
over the same sine and cosine. I guess the loop came from one of these changes, as
I cannot trace it back to the current code. The part from the current C++ code in
celestrak is around line 1060 in sgp4unit.cpp, and there are no loops. So I would
say these loops must be removed.

Could you open a bug report for this in the forge?

Thanks for spotting this!

Luc