Skip to content

Conversation

@giordano
Copy link
Member

@giordano giordano commented Nov 5, 2025

Diff better reviewed by ignoring whitespace changes.

@giordano
Copy link
Member Author

giordano commented Nov 5, 2025

Oof, Aqua tests are failing because the OpenMPI dependency introduced in #296 is actually unused (and it should be, that was a hack...)

@giordano giordano force-pushed the mg/aqua branch 3 times, most recently from 3eeaf24 to caee01d Compare November 5, 2025 15:29
@giordano
Copy link
Member Author

giordano commented Nov 5, 2025

Uh uh, Aqua detected an(other) actual issue:

Possible type-piracy detected:
[1] ldiv!(A::PDMat, B::AbstractMatrix) @ ParticleDA ~/work/ParticleDA.jl/ParticleDA.jl/src/filters.jl:191
Piracy: Test Failed at /Users/runner/.julia/packages/Aqua/MCcFg/src/piracies.jl:245
  Expression: isempty(v)
   Evaluated: isempty(Method[ldiv!(A::PDMat, B::AbstractMatrix) @ ParticleDA ~/work/ParticleDA.jl/ParticleDA.jl/src/filters.jl:191])

# ldiv! not currently defined for PDMat so define here
LinearAlgebra.ldiv!(A::PDMat, B::AbstractMatrix) = ldiv!(A.chol, B)

src/filters.jl Outdated
# avoid unnecessary allocations.
observation_buffer .-= observation
ldiv!(cov_Y_Y, observation_buffer)
ldiv!(cov_Y_Y.chol, observation_buffer)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Suggested change
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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link

codecov bot commented Nov 5, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.49%. Comparing base (c101d0d) to head (e3a147f).
⚠️ Report is 2 commits behind head on main.

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@giordano giordano merged commit 523f3e5 into main Nov 5, 2025
15 of 16 checks passed
@giordano giordano deleted the mg/aqua branch November 5, 2025 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants