debug: fallback if MovingHorizonEstimator arrival covariance update fails
#151
Conversation
franckgaga
changed the title
MovingHorizonEstimator arrival covariance update failsMovingHorizonEstimator arrival covariance update fails
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #151 +/- ##
=======================================
Coverage 98.88% 98.89%
=======================================
Files 24 24
Lines 3766 3789 +23
=======================================
+ Hits 3724 3747 +23
Misses 42 42 ☔ View full report in Codecov by Sentry. |
|
As a fallback for the MHE, I first tried to implement the method of
described here on page 5-3. The eigenvalues of the resulting matrix are indeed all >= 0, but the I finally opt to keep the arrival covariance of the last iteration I'll add the Joseph form for the |
There was a problem hiding this comment.
This approach is fine by me, the warning indicates what's being done. There are factorizations that can handle semi-definiteness, such as https://github.com/timholy/PositiveFactorizations.jl, but in practice I don't think it'll matter much as you say in #98 (comment)
src/estimator/mhe/execute.jl
Outdated
| if err isa PosDefException | ||
| @warn("Arrival covariance is not positive definite: keeping the old one") | ||
| else | ||
| rethrow(err) |
There was a problem hiding this comment.
yes true thanks! That's what I normally do but the copilot wrote this line for me. The more I use the copilot, the more I find mistake like that.
|
Keep in mind that you can get julia> cholesky([NaN;;])
ERROR: PosDefException: matrix is not Hermitian; Factorization failed.
Stacktrace:
[1] checkpositivedefinite(info::Int64)we don't want to "hide" this issue by the |
src/estimator/mhe/execute.jl
Outdated
| if err isa PosDefException | ||
| @warn("Arrival covariance is not positive definite: keeping the old one") | ||
| else | ||
| rethrow(err) |
There was a problem hiding this comment.
yes true thanks! That's what I normally do but the copilot wrote this line for me. The more I use the copilot, the more I find mistake like that.
the `Union` type was overly complex and `getinfo` should not be used in "normal" operation, only for troubleshoothing. The computational performance are not that important (there is already many allocations in this function)
The full output without ellipsis can still be shown in the REPL with the following logger: `debuglogger = ConsoleLogger(stderr, Logging.Debug, show_limited=false)`
FYI, It does not generate this exception when the matrix is a julia> cholesky(Hermitian([1 NaN;NaN NaN], :L))
Cholesky{Float64, Matrix{Float64}}
L factor:
2×2 LowerTriangular{Float64, Matrix{Float64}}:
1.0 ⋅
NaN NaNNo exception at all is thrown as you can see. But the NaNs are propagated correctly so It will presumably crash later in the code. |
|
I just had a related idea, when I write optimization over positive definite matrices, I usually optimize the triangular part of a cholesky factor instead of over the matrix itself, example here, see the functions n2 = length(params) ÷ 2
Qchol = triangular(params[1:n2])
Rchol = triangular(params[n2+1:2n2])
Q = Qchol'Qchol
R = Rchol'RcholThis would be equivalent to using the "square-root form" of a Kalman filter, but for optimiztion-based approaches it's much simpler to set up. Not sure if it's relevant in the current case of non-pos-def arrival covariance in MHE, but if it is, it could perhaps give a boost to performance. |
|
Oh interesting! |
context: See #98 (comment) point 1.