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.