I just pushed a new branch (https://gitlab.orekit.org/orekit/orekit/-/commits/performance) to integrate the new Hipparchus features (i.e. Gradient, etc.) in Orekit. I performed the changes for the different measurement types and also for the different DS converters. You will see that I declared the old DS converters (i.e. DSConverter, DSSTDSConverter, TroposphericDSConverter and IonosphericDSConverter) as deprecated and I replaced them by new ones (i.e. NumericalGradientConverter, DSSTGradientConverter, TroposphericGradientConverter and IonosphericGradientConverter).
After doing that and before finishing the the transition, I performed some tests on the Orekit orbit determination. Using Gradient instead of DerivativeStructure/DSFactory does not change the accuracy of the tests. That’s a good new!
However I saw some performance issues.
Here the duration of the three Orekit OD tests before performing the modifications:
W3B OD: 38.739 seconds
GNSS OD: 25.904 seconds
Lageos OD: 65.544 seconds
Here the duration of the three same tests using Gradient instead of DSFactory and DerivativeStructure:
W3B OD: 135.964 seconds
GNSS OD: 38.208 seconds
Lageos OD: 92.618 seconds
I used Yourkit tool to see what happened. I used it on the worst case: W3B OD.
For non-Yourkit users, the main difference between the two runs and the cause of increased computation time is in HolmesFeatherstoneAttractionModel.gradient(...) method. Precisely, according to Yourkit tool, the cause of the problem is the performance of the Gradient.multiply(Gradient) method. Yourkit highlights the line 235 of the Gradient class (i.e. result.grad[i] = MathArrays.linearCombination(grad[i], a.value, value, a.grad[i]);)
2 last informations:
The performance problem is not affecting DSST orbit determination.
The performance problem occurred just after the migration of the DS converter classes (i.e. after changing the measurement classes, performance where fine).
If the accuracy is the same but performance is now 50-250% slower why is this change being made? Are there optimizations that we believe will make it perform faster or with higher accuracy that haven’t been integrated yet? The speed of the OD is slower than I was expecting when I experimented with it last month for heliocentric stuff (still have some work to do on that). Making it 1.5-3.5x slower would exacerbate that.
According to Luc’s remark (Next Orekit version), use Gradient instead of DSFactory could improve OD performance in terms of computation time. Not in terms of accuracy. That’s why we perform the change (Not in the official repository but in a dedicated branch). However, that is not the case. OD is slower than before the modification. The gap is important: 3.5x for W3B OD. According to Yourkit tool the problem is in Gradient.multiply(...) method which take a lot of time compared to DerivativeStructure implementation.
The idea of the patch is to use naive implementation instead of linear combination. The use of linear combination was perhaps overkill for these methods and DerivativeStructure does not use it there.
@bcazabonne HFAM.acceleration(Field…) has a fast path and a slow path. It is probably worthwhile to make sure it is still using the fast path. If so HFAM shouldn’t be calling multiply at all.
Performance are the same for GNSS OD but better for Lageos 2 OD. However, I still have a problem with W3B OD (solution below).
I also performed some tests on a 75k measurements orbit determination (GPS orbit type). Here the results:
Using DSFactory/DerivativeStructure: 159.647 s
Using Gradient (before the patch): 214.743 s
Using Gradient (after the patch): 146.032 s
It shows that using using Gradient (and the patch) instead of DSFactory/DerivativeStructure can improve a little bit the performance.
+1 for your remark. It helps me a lot. Before the modification fast path was used. Now, slow path is used. I see that the same problem occurs for the DragForce, the slow path is used instead of the fast path.
The problem is that we are using Gradient object. However, the method isStateDerivative(...) of both HFAM and DragForce models cast the Field to a DerivativeStructure. Therefore, an exception is thrown and the methods returns automatically false instead of true. Here the code for HFAM model:
private <T extends RealFieldElement<T>> boolean isStateDerivative(final FieldSpacecraftState<T> state) {
try {
final DerivativeStructure dsMass = (DerivativeStructure) state.getMass();
final int o = dsMass.getOrder();
final int p = dsMass.getFreeParameters();
if (o != 1 || (p < 3)) {
return false;
}
@SuppressWarnings("unchecked")
final FieldPVCoordinates<DerivativeStructure> pv = (FieldPVCoordinates<DerivativeStructure>) state.getPVCoordinates();
return isVariable(pv.getPosition().getX(), 0) &&
isVariable(pv.getPosition().getY(), 1) &&
isVariable(pv.getPosition().getZ(), 2);
} catch (ClassCastException cce) {
return false;
}
}
Perhaps we could introduce at Orekit level a marker derived class, say StateGradient that would extend Gradient. Then this class would be used ig and only if the gradient first three derivations parameters are known to be the position and if present the next three derivations parameters are known to be the velocity. We could then have isStateDerivative also check for this specialized class.
For this to work, I would need to add a way for Hipparchus to automatically build instances of user-defined derived classes instead of its own vanilla Gradient. This could be done by using the same trick as in the Complex class where a protected factory method is used and can be overriden by users.
I will do this this afternoon so it should be available soon so you can try it. I will include in the unit tests some example classes that you could directly copy and rename (basically, these classes will only define constructors and factory methods, nothing else).
So now we can say that using Gradient instead of DSFactory/DerivativeStructure improves a lot the orbit determination performance in terms of computation time. In terms of accuracy, no changes (but that’s normal). Improvements are also present for the DSST OD!
Finally, looking at the 75k measurements OD improvements are also present:
Using DSFactory / DerivativeStructure : 159.647 s
Using Gradient (and the different corresctions) : 135.99 s
I choose that solution because the method isStateDerivative(...) returns false if the cast is not possible. If the cast is possible, the method returns false if the order is not equal to 1. To my mind, this is equivalent to replace the DerivativeStructure object by a Gradient object.
I didn’t run all Orekit tests, just the main ones: OD, partial derivatives, propagation and force models. They all work fine.
This fix looks good to me. There is no need for a dedicated class.
Perhaps we should keep the tests with DerivativeStructure temporarily as deprecated methods? I know the methods are private but the acceleration method is public so users could already call it with DerivativeStructure. In this case, we should have two methods: isDSStateDerivative and isGradientStateDerivative.
I have pushed the fix avoiding calls to linearCombination to Hipparchus master (for both Gradient and FieldGradient).
That is a speedup of 1.6 for W3b, 1.2 for GNSS, 1.5 for Lageos. Those seem like some nice improvements.
For the usability issues would it be reasonable to make DerivativeStructure a sub type of Gradient since it can do everything that Gradient can do and more?
I doubt it can be done easily, at least for the field version. The way parameterized types work in Java is black magic to me and type reification doesn’t help. We also have an over-complicated setup with fields in Hipparchus. Between the getField, getRuntimeClass, DSFactory, FDSFactory and all that stuff, I was never able to find a way the compiler accept what seemed logical to me. I was not even able to set up a common top level abstract class above UnivariateDerivative1 and UnivariateDerivative2 (I would have liked something like UnivariateDerivative2 extends UnivariateDerivative1 and UnivariateDerivative1 extends AbstractUnivariateDerivative). If I remember well, either getRuntimeClass would not compile, or Field<T> getField() from FieldElement would not compile, or the type would fail at runtime. I tried with some variations like <T super RealfieldElement>, or parameterizing with two types, one for the element and one for the field, nothing worked.
If you find a way to solve this, I would be happy to see it. I think that for Hipparchus 2.0 we should simplify as much as possible this mess.
yes, it is a limitation of Java that interfaces cannot be inherited with different type arguments. But you could add another another superinterface that extends RFE, e.g. Derivative<T extends Derivative<T>> extends RFE<T> that would have methods to query number of parameters, differentiation order, retrieve derivatives, etc. Then code could be written to use Derivative<?> to allow different implementations.
It would be similar to how AbstractDetector works.
I’ve been thinking about it. I’m thinking of not declaring them depreciated but keeping them. I’m going to duplicate them in order to have their implementation using the Gradient class. So we validate the new use with Gradient and we keep the validation with DerivativeStructure to prove that we remain compatible with a minor version and also to prove that it still work with DerivativeStructure.
On the other hand, I can open an issue for 11.0 saying that we have to remove this duplication (in the tests and in the code). As I performed the change, I will assign the issue to me.
