I noticed something strange in the UKF implementation. When the process noise is called (in UnscentedKalmanModel), the two states seem to always have the same date, unlike with the extended Kalman filter. This is also in contradiction with what process noise is basically, because any model based on \Delta t will be constant here. I noticed there was this in the code: TODO: predictedSpacecraftStates are not valid here
(just before getNormalizedProcessNoise) which disappeared in a commit, but I don’t think it actually got solved. @markrutten I know you recently refactored things, is it something that was unforturnately overlooked in the process?
Yep, I’m definitely to blame here @Serrof! My refactoring didn’t change the process noise behaviour of the UKF, but it did uncover the difference with the EKF that you’ve noticed. You’re correct, that TODO is still relevant … that part of the algorithm wasn’t changed, mainly because I’m not really sure how to approach it. Apologies, it wasn’t my intention to leave the UKF in a broken state.
No worries Mark. Anyway you say that the behaviour was similar before right? So you didn’t introduce any regression.
Why are you unsure about how to fix the problem? Is it purely code architecture or there is something else?
It’s a code architecture problem, not an algorithm problem. Comparing with the EKF at the point where the predicted states are constructed, in the UKF model we have access to the predicted sigma-points, not the mean/covariance of the predicted states (which are formed inside the Hipparchus UKF). This is what I wrote in the merge request:
I left a “TODO” in the UnscentedKalmanModel’s getEvolution to flag that the predicted states aren’t correct when calling getProcessNoise.
I have a couple ideas of how to fix this, but they both involve breaking the API:
Pass the unscented transform provider into the model and explicitly extract the predicted state mean from the sigma points. This feels like a bit of a hack (we’d be duplicating what happens inside the UKF), but it doesn’t change user-facing parts of the API.
Change the Hipparchus UnscentedProcess to add an extra method, e.g. separate getEvolution into getPredictedStates and getProcessNoiseCovariance, or something similar. I think this is preferable but requires an API change in Hipparchus.
The Hipparchus change is a modification to the API, so needs to wait for a major release.
I’ve suggested simply removing the noise calculation from getEvolution in UnscentedProcess (so the UnscentedEvolution doesn’t contain the process noise matrix anymore) and introducing a new method that has the signature:
RealMatrix getProcessNoiseMatrix(double previousTime, RealVector predictedState, T measurement);
The idea was to make the predicted state (and time) available to the process noise calculation. That’s all that’s needed for the orekit modification. But we could potentially make all of the predicted sigma-points (i.e. an UnscentedEvolution) available as well as the predicted state and possibly instead of the measurement, since UnscentedEvolution also contains the time.
Thank a lot for taking care of this.
Since you’re touching the API maybe we could add stuff useful for smoothers as several users have asked for guidance on how to implement it in Orekit. It’s just a suggestion though, I think we really need the process noise issue fixed for 13.0 so I don’t want to delay the MR.
I started poking at the Hipparchus part of smoothing today, which (I hope) will be reasonably straightforward. I expect orekit will need a more significant change/addition, but I hope to have some time over Christmas break.