I’ve noticed some performance issues with the class AggregateBoundedPropagator.
I couldn’t understand why an AggregateBoundedPropagator containing multiple IntegratedEphemeris was much slower than a single IntegratedEphemeris.
It turns out the problem comes from the method AggregateBoundedPropagator.basicPropagate.
At the beginning of this method, if we replace:
final SpacecraftState state = getPropagator(date).propagate(date);
With
final BoundedPropagator propagator = getPropagator(date);
final SpacecraftState state = propagator instanceof AbstractAnalyticalPropagator ?
((AbstractAnalyticalPropagator) propagator).basicPropagate(date) :
propagator.propagate(date);
The performance of the multiple-ephemeris propagation is on par with the single-ephemeris propagation.
The idea is to use the fact that if the propagator is analytical then we can use basicPropagate instead of propagate.
Does that look legit to you? Or is there any caveat that I missed in the difference between propagate and basicPropagate here?
It propagates a LEO numerical propagator for 300 days with a step of 60s step and do:
A single ephemeris generation
A multiple ephemeris (each orbit) generation
Repropagates 1 with a handler (every 3s to force a long time)
Repropagates 2 with the same handler
Without the fix (4) is about 50% slower than (3).
With the fix (4) and (3) takes about the same time.
This test doesn’t fully show performance improvement. I’ve noticed running times divided by 5 with the fix when running something more complex with many low-thrust maneuvers added to the underlying propagator.
Do I have your approval to implement this fix in Orekit?
Thanks for noticing this. Your proposal makes sense. Actually I wonder if the current implementation is sound at all. Because any call to propagate means that the events detection is active, but basicPropagate does not include any normally. This would mean that with a mix of analytical and non-analytical wrapped propagators, we’d get a weird behaviour am I right?
With the current implementation no, but with the one I propose, I realize that yes…
But yes it’s weird to do a complete propagate in basicPropagate since it’s not supposed to be used that way. That’s the reason why it’s underperforming in the first place.
I tried yesterday to launch the tests with the fix and I ran into problems in SP3ParserTest (for example, testSP3Propagator) because the additional states are not managed with basicPropagate.
So, to fix the tests, I had to make method AbstractAnalyticalPropagator.updateAdditionalStates public and replace the code from my first post with:
// do propagation
final BoundedPropagator propagator = getPropagator(date);
SpacecraftState state;
if (propagator instanceof AbstractAnalyticalPropagator) {
// Take advantage of the fact that the underlying propagator
// is analytical to speed up propagation
final AbstractAnalyticalPropagator analyticalPropagator = (AbstractAnalyticalPropagator) propagator;
state = analyticalPropagator.basicPropagate(date);
state = analyticalPropagator.updateAdditionalStates(state);
} else {
state = propagator.propagate(date);;
}
But as you say we will probably have unexpected behavior with event detection…
So I don’t know, should I still open the issue and wait for when we manage to provide a consistent fix?
I’m pretty sure some inheritors already have basicPropagate public actually. However this is API breaking. For users inheriting from AbstractAnalyticalPropagator I mean. So we might need to wait for a major release I’m afraid.
