Conversation
|
Oof, Aqua tests are failing because the OpenMPI dependency introduced in #296 is actually unused (and it should be, that was a hack...) |
3eeaf24 to
caee01d
Compare
|
Uh uh, Aqua detected an(other) actual issue: Lines 190 to 191 in c101d0d |
src/filters.jl
Outdated
| # avoid unnecessary allocations. | ||
| observation_buffer .-= observation | ||
| ldiv!(cov_Y_Y, observation_buffer) | ||
| ldiv!(cov_Y_Y.chol, observation_buffer) |
There was a problem hiding this comment.
@matt-graham I'm just inlining the method I removed above, without explicitly defining a method which commits type piracy. Does it sound good? Tests pass locally for me with this change.
There was a problem hiding this comment.
Yeah can't really remember why I explicitly defined method here as it looks like cov_Y_Y will always be a subtype of AbstractPDMat, but I suspect I was originally trying to allow for cov_Y_Y being of other abstract matrix types and wasn't aware of type piracy issues. Change looks fine - possibly
| ldiv!(cov_Y_Y.chol, observation_buffer) | |
| ldiv!(cholesky(cov_Y_Y), observation_buffer) |
might be better to make code more generic / not explicitly access chol field? But this is tangential to change here.
There was a problem hiding this comment.
Right, that's actually similar to what they do in PDMats: https://github.com/JuliaStats/PDMats.jl/blob/5046635eba5a44901b7cec0ca68c48cf8ed967d9/src/pdmat.jl#L129, but they use also internal functions chol_lower/chol_upper
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #300 +/- ##
==========================================
- Coverage 94.50% 94.49% -0.01%
==========================================
Files 9 9
Lines 655 654 -1
==========================================
- Hits 619 618 -1
Misses 36 36 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Co-authored-by: Matt Graham <matthew.m.graham@gmail.com>
Diff better reviewed by ignoring whitespace changes.