Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use cholesky decomposition in correlated_values and correlated_values_norm #103

Closed
wants to merge 9 commits into from

Conversation

ces42
Copy link

@ces42 ces42 commented Aug 1, 2019

This uses a Cholesky decomposition instead of diagonalization in correlated_values and correlated_values_norm. The idea is the same as presented in #99.

However, dealing with degenerate covariance matrices proved to be a bit tricky, since numpy refuses to do a Cholesky decomposition of a matrix that is only positive semi-definite. Therefore I added a function that performs a LDL decomposition in those cases (this would also be available in scipy).

Local tests (and theory) suggest that this should be more precise and faster than the old implementation.

Both correlated_values and correlated_values_norm now raise an error if the covariance/correlation matrix fails to be (nearly) positive semi-definite.

@jagerber48
Copy link
Contributor

I want to make this change, but I think this PR is too old to do so. I think it would be challenging to merge it in. I'll explore a new PR that lifts the main changes from this PR. @ces42 if you're still interested in this you should feel free to open a new PR also.

@newville
Copy link
Member

newville commented Nov 4, 2024

@jagerber48 @ces42 I'm +1 on trying to resurrect this.
I might say both that a) it is not the highest priority, and b) it should not have to wait another 5 years ;).

@ces42
Copy link
Author

ces42 commented Nov 4, 2024

I could try to make an analogous PR again. The main issue still is that numpy's Cholesky decomposition does not work in the semi-definite case, so some kind of fallback is necessary (and creating the fallback will probably be more work than the "general case"). I can see 3 options for the fallback:

  1. Use the LDL decomposition from scipy. This would add scipy as a dependency.
  2. Use diagonalization as in the current version of uncertainties. This is slow and numerically imprecise.
  3. Hand-code a LDL (or similar) decomposition, as I ended up doing in this PR. This is slow and adds a lot of code.

It would be possible to detect whether scipy has been imported and only do 1. in that case. This might be considered a bad practice though...
Any thoughts on what approach might be best?

@jagerber48
Copy link
Contributor

I might say both that a) it is not the highest priority, and b) it should not have to wait another 5 years ;).

@newville
Yes, the reason these blocks of code are popping up is because I think they're going to need to be worked on for the large AffineScalarFunc refactor but I don't want to either

  • rewrite them using their same logic or
  • also load the transition to Cholesky decomposition into that PR

so I'm exploring knocking the Cholesky decomposition out first.


@ces42

Any thoughts on what approach might be best?

  1. Use diagonalization as in the current version of uncertainties. This is slow and numerically imprecise.

I like this option a lot. The semi-definite case would be something like you have a covariance matrix of random variables, but there is some basis in which one or more of the random variables has zero variance? It's a literal edge/degenerate case that hadn't occurred to me but I'm glad you point it out. Since it is an edge case I think it's no problem if users take a performance hit when they hit that case.

  1. Use the LDL decomposition from scipy. This would add scipy as a dependency.

We're wringing our hands in some other threads about having numpy as an optional or required dependency. I think appetite would be pretty low for adding scipy.

  1. Hand-code a LDL (or similar) decomposition, as I ended up doing in this PR. This is slow and adds a lot of code.

If it is slow what is the advantage of this approach over 2.? Maybe it's faster than 2.but slower than 1.? Seems like we could start with approach 2. then evolve to 3. if there's demand for more performant decompositions on these semi-definite cases.

It would be possible to detect whether scipy has been imported and only do 1. in that case. This might be considered a bad practice though...

I don't think this is a bad practice. It would be a little bit clunky but the logic for semi-definite matrices could be something like:

  • exception if numpy is not importable
  • eigenvalue decomposition if numpy is importable but not scipy
  • scipy Cholesky decomposition if scipy is importable

For positive-definite it would be

  • exception if numpy is not importable
  • numpy Cholesky decomposition if numpy is importable

While the above is kind of appealing, I'm personally tempted to say just leave scipy out of it as a first pass. For years it's been using the slow numpy eigen decomposition and it's seemingly been ok for users. Users can always spin their own function using something else if they have a more performant way to do the decomposition and if that matters for them. We can always add the greedy (i.e. taking advantage of the scipy resource if available) performance enhancement down the road.


Be aware of this PR that is targeting these same functions and shows some of the discussion around optional dependencies. #268

@jagerber48
Copy link
Contributor

jagerber48 commented Nov 16, 2024

Moving forward on this at #271. @ces42 I'm curious if you have any comments on that PR.

@jagerber48 jagerber48 closed this Nov 16, 2024
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