fixed cholesky error in simulate #314
Conversation
Codecov Report
@@ Coverage Diff @@
## master #314 +/- ##
==========================================
- Coverage 91.01% 90.77% -0.24%
==========================================
Files 35 35
Lines 3296 3308 +12
==========================================
+ Hits 3000 3003 +3
- Misses 296 305 +9
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
| if isposdef(sys.Q[1]) | ||
| chol_Q = cholesky(sys.Q[1]) | ||
| elseif ispossemdef(sys.Q[1]) |
There was a problem hiding this comment.
Before this change, if for any reason Q is not positive semidefinite, we would get an error at cholesky which would be clear to the user, but now it will just skip the cholesky step entirely which might result in more confusing errors. Might be good to either change this elseif to else, or have a fallback that throws an error message.
There was a problem hiding this comment.
What do you think about adding an else that gives an error if Q is not positive definite or semidefinite?
There was a problem hiding this comment.
Yeah that makes sense to me!
| chol_Q.L[:, :] = round.(chol_Q.L; digits = 6) | ||
| chol_Q.U[:, :] = round.(chol_Q.U; digits = 6) | ||
| chol_Q.UL[:, :] = round.(chol_Q.UL; digits = 6) |
There was a problem hiding this comment.
Do we have reasons to be concerned about positive numerical imprecisions that can be magnified in the estimation process because of this rounding? Can we use more digits perhaps?
There was a problem hiding this comment.
I'll perform some more tests to check if this brings an important impact. Initially I did this rounding just to be consistent to the original Q matrix, but I guess it might not affect the results significantly.
There was a problem hiding this comment.
The process of rounding only affects the estimated scenarios/quantiles. When ingnoring it, there are some differences in the calculated quantiles. What do you think about just increasing the number of digits, as you suggested before? Maybe 10 digits?
There was a problem hiding this comment.
Increasing the digits could be enough, let's try 10
|
Thanks 🙂 |
|
Can you also bump the package version so we can make a release with this change? Thanks |
An error occurs when performing cholesky in 'simulate' functions.
When there is at least one element of the diagonal of sys.Q equal to 0.0, the matrix will not be positive definite and the cholesky decomposition won't work.
To bypass this issue, when sys.Q is positive semidefinte (i.e., there are elements of the diagonal of sys.Q equal to 0.0), we add a small number and perform cholesky without errors. Then, we round the elements of the decomposed matrix to recover the original 0.0 values.