I’m back with another performance-related query.
In NumericalPropagator, the default OrbitType is Equinoctial and the default PositionAngleType is True. However, when we look into computeJacobianTrueWrtCartesian, we see that it encapsulates calls to the other two angles. This can lead to non negligible computational overhead. Why not use Mean as the default type? This would have the best performance.
I know that the user can set what they want, but I don’t think people are always conscious of “under the hood” stuff like that.
Bonus question: why are the Gauss equations not explicitly implemented? Instead the Jacobian w.r.t. Cartesian is computed at each integration step. Ok there’s less code to maintain but surely there would be algebraic simplifications that would speed things up no? I guess we’d need a prototype to know for sure.
It’s historical, and it is a bad choice, indeed from a numerical point of view mean anomaly is smoother and easier to integrate (well… it is linear with respect to time). +1 for the change.
We changed it years ago. The reason was that Gauss equations in some sets of parameters are not always available. And in fact, Gauss equations are Jacobians with Keplerian part removed (which is zero for all parameters except anomaly), even if it is not the traditional approach.
If we intend to add new orbital parameters (and this may happen because of CCSDS), we will anyway need the equivalent of Gauss equations/Jacobians and I prefer to maintain only one approach.
I’ve opened an issue for the default PositionAngleType, but it’s breaking at least 60 tests…
I think it would be nice to have explicit Gauss equations for Keplerian and equinoctial elements in Orekit. They’re used a lot, at least in the literature. They are more or less a Jacobian matrix yes (there’s also a frame transformation as they are usually in a local frame). I understand that implementing them for present and future OrbitType would be a pain. But we could have a default method that uses the Jacobian w.r.t. Cartesian (which would remain mandatory) actually, and only some inheritors would overwrite it with explicit algebraic expressions. Anyway there would be a bit of work, so I don’t know.
Actually, if one sets the PositionAngleType to MEAN for example, the OsculatingStateMapper still uses TRUE for some reason and thus time is then lost making conversions in mapArrayToState. That looks like an undesired behaviour doesn’t it?
Edit: even if the StateMapper was using MEAN, I think the constructor of EquinoctialOrbit would make the conversion anyway. So we have two opposite behaviours here: MEAN is the fastest Jacobian computation (because time is the independent variable for integration) but TRUE is the fastest Orbit creation (by design, as the class systematically converts onto this PositionAngle).
I’m getting back to this now that on develop branch (Field)Orbit does not automatically convert position angles to TRUE.
I’m still doing some profiling but I think we ought to change the default variable in (Field) NumericalPropagator to ECCENTRIC. It looks like a good compromise as MEAN has the fastest Jacobian but getPosition is faster with ECCENTRIC, at least for EQUINOCTIAL.
When dealing with performance issues, I prefer to stick with profiling with tools rather than trying to explain that this equation is faster than that equation. Intuitive guesses are almost always false, so I just trust tools there.
If you observe ECCENTRIC is better overall (say on some heavy duty use like orbit determination), then go to ECCENTRIC.
I wonder if this change shall be done in a major release and not in a minor release…
It will not change the API, but probably the outputs of the generated orbits
It’s already on develop
Btw it comes with some other performance improvements.
Anyway it’s very easy for the user to keep their old result: propagator.setPositionAngleType(PositionAngleType.TRUE)
I did not modify the method tolerances on purpose to avoid more numerical changes.
Yes, but user will have regressions in their operational systems after updating the Orekit version. To have no regression, they will have to update their code. That’s why I think this should be in a major release.
I remember an old discussion were we made a mistake by releasing an important output change in a minor release: Incompatibility in 10.3 - #3 by bcazabonne
That’s why now I prefer being careful with important output changes, especially with big features like propagation.
The PositionAngleType is an attribute that has TRUE as default value in the current release. So in theory, from a software development point of view, Orekit users should set it manually in their application, otherwise they do expose themselves to regression with new releases. I hope it’s common practise in operational context especially…
Anyhow, I understand your concern and ultimately I’m not opposed to maintain the current default value until Orekit 13. I’d like people to know though that with recent changes in the code, ECCENTRIC is the best choice performance-wise.
Out of curiosity, what is the performance improvement you observed when using ECCENTRIC instead of TRUE PositionAngleType, e.g. for orbit determination? Is it a few percent or tens of percents?
Regarding the specific modifications around the PositionAngleType alone we’re talking a few percents for propagation itself, it really depends on the perturbations of course.
I’ve not done any timings for OD, however given other performance boosts in Orekit and Hipparchus, I wouldn’t be surprised if the gains increased a bit.
By the way, I would appreciate some users insight on the change in default PositionAngleType for the next minor release. Do some people share the same reservations than Bryan?
Thanks for your answer. I also asked because I understood from your posts that MEAN is better for Jacobian, while ECCENTRIC is better for position, so I wondered what the overall improvement is. If I find some time, I’ll try myself as well.
I do share the same reservations as Bryan. I also think that this can cause regressions, or at least different results and thus failing tests.
Settings like OrbitType and PositionAngleType in for example NumericalPropagator are optional. One has to explicitly call the setter to change the default settings. I don’t expect most users to do that. Of course, we can expect advanced users to follow good coding habits, but we all know the practice Code is never perfect.
If I didn’t get it wrong, you noted in a previous post that 60 unit tests failed after changing the default PositionAngleType. So I would expect the same (or worse) for other codes based on Orekit. So I’d say that a major release is a better moment to introduce such a change.
Code is never perfect.