Open aplavin opened 2 years ago
Merging #81 (3e6b6ea) into main (eb6999e) will increase coverage by
0.09%
. The diff coverage is100.00%
.
@@ Coverage Diff @@
## main #81 +/- ##
==========================================
+ Coverage 88.62% 88.72% +0.09%
==========================================
Files 13 13
Lines 554 550 -4
==========================================
- Hits 491 488 -3
+ Misses 63 62 -1
Impacted Files | Coverage Δ | |
---|---|---|
src/bounds/ellipsoid.jl | 85.88% <100.00%> (+0.48%) |
:arrow_up: |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update eb6999e...3e6b6ea. Read the comment docs.
Pushed the second commit that tries to make a proper fix.
Hey @aplavin thanks for the work on this, sorry I haven't had time to respond to any of this yet, I'm swamped under my thesis work nowadays. I've been wanting to rewrite the bounding space algorithms for a while now, I think they're the slowest part of the NS code (not sure the impact on total performance, but the fits have tons of allocations and use mutable structs and are pretty haphazard). I don't really like the interface, either, and I think it can all be rewritten to utilize Distributions.jl since these "bounding spaces" are just statistical distributions.
So, with all that being said, I just want you to know that I've seen this, and that I have ideas for making this part of the interface better! I really appreciate what you've contributed already 😃
Yeah, no real hurry. I've used my modifications (this PR) on some problems already, and sampling basically never fails - as in "throws", while it did before. Note that the PR is independent from the exact interface, it's about computations themselves.
upd: this post describes background and the first commit of this PR
It's probably more reasonable? But still not a complete fix, see below.
I encountered an error arising from the
A
matrix here having (kinda) negative eigenvalues: https://github.com/TuringLang/NestedSamplers.jl/blob/eb6999e0da004524a5848eb2c99b1f53e8a57f1d/src/bounds/ellipsoid.jl#L108 This matrix is passed to theEllipse
constructor, and fails insqrt
at https://github.com/TuringLang/NestedSamplers.jl/blob/eb6999e0da004524a5848eb2c99b1f53e8a57f1d/src/bounds/ellipsoid.jl#L49 Actually, eigenvalues ofA
are not negative as-is, but become negative inSymmetric(A)
. Digging into this error, I found that the https://github.com/TuringLang/NestedSamplers.jl/blob/eb6999e0da004524a5848eb2c99b1f53e8a57f1d/src/bounds/ellipsoid.jl#L168-L178 function applies a huge correction to the cov matrix in my case at https://github.com/TuringLang/NestedSamplers.jl/blob/eb6999e0da004524a5848eb2c99b1f53e8a57f1d/src/bounds/ellipsoid.jl#L105 Not sure why it is so, and I don't understand themake_eigvals_positive!
function, what exactly should it do and why? Maybe, adapting the function https://github.com/joshspeagle/dynesty/blob/2c5f1bbe5a0745c6625876f23ec6aa710c845fd4/py/dynesty/bounding.py#L1230 from dynesty would be better?This PR fixes the error with the particular matrix (below) because it has the condition number < 1e10. But I don't think it's a proper solution.
To reproduce my original error in
fit()
: