search

pints-team / pints

Probabilistic Inference on Noisy Time Series
http://pints.readthedocs.io
Other
226 stars 33 forks source link

Make sure MultimodalNormalLogPDF throws an error when supplying non-posdefinite cov matrix #498

Open ben18785 opened 6 years ago

ben18785 commented 6 years ago

The populationMCMC notebook has this, I think.

MichaelClerx commented 6 years ago

If it does that'll be from the underlying scipy / numpy code then (so not something we need to test)

ben18785 commented 3 years ago

I don't understand why the below returns a non-zero number:

dist = scipy.stats.multivariate_normal(mean=[16, 12], cov=[[0.8, 0.2], [0.1, 1.4]])
dist.pdf([3, 4])

here cov is not symmetric, so does not satisfy, "The covariance matrix cov must be a (symmetric) positive semi-definite matrix" in the scipy docs...

This looks like a bug in scipy to me! So, we need to test.

ben18785 commented 3 years ago

Wrote a bug issue on Scipy: https://github.com/scipy/scipy/issues/13541

MichaelClerx commented 3 years ago

I'm happy for us to stick in a test that the matrix must be symmetric, although we'll run into the precision issues mentioned by the scipy devs.

ben18785 commented 3 years ago

It's fine for us not to test it in that case (I do think the scipy reasoning is a bit weak though: if you're afraid of numerical round off, fine, but most other libraries manage to find a sensible middle ground). Although we need to make sure that we use the method properly, so need to fix the populationMC notebook (and more generally check wherever MultiModalLogPDF is called). I'll create a separate ticket for this though and close this one.

On Wed, Feb 10, 2021 at 8:33 AM Michael Clerx notifications@github.com wrote:

I'm happy for us to stick in a test that the matrix must be symmetric, although we'll run into the precision issues mentioned by the scipy devs.

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/pints-team/pints/issues/498#issuecomment-776536220, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABCILKBY3KWPPUPZZ22H22TS6JAGLANCNFSM4FTAODEQ .

ben18785 commented 3 years ago

Actually, I'm now less sure: we do have such a test in the GaussianLogPDF, so I'm just going to grab this from there.