Repository navigation
ENH: Multivariate normal speedups - #14197
Conversation
…ultivariate_normal
charris
added
01 - Enhancement
component: numpy.random
09 - Backport-Candidate
|
I have mixed feeling about putting a @zoj613 Enhancements like this need a post on the mailing list for discussion and a release note. The release not process has changed, so I'll let @seberg explain it. @bashtage I haven't checked, but |
|
@charris I tried using the mailing list given as |
|
@zoj613 Oh, that needs fixing. The link is to a list of python.org mailing lists :) Just use |
|
I want to move things a bit around. So if this is still settling, maybe hold off on the release notes for a bit. Right now, you would add an It should then be formatted as: (but if you are quick, just go ahead and I will fix it up/move things around as necessary, the small issue is that right now towncrier does not really support the way we write our release notes). |
|
I went ahead and posted to the list. |
Thanks a lot. sorry about that. I haven't had the chance to push the required changes until now. |
|
I think you are currently editing |
|
I am getting errors while running the tests on macOS after merging the latest changes from the master branch. I get a |
should I then leave the function in |
|
@zoj613, probably, although you could leave the change for now and just change the mode default to |
|
I'm not really sure I like including So I think I prefer a spelling closer to the earlier revision,
Another option would be
(precedent: |
|
If the covariance matrix is numerically semidefinite, cholesky will trip up. But isn't this breaking backwards compatibility where SVD was working but cholesky will fail? |
With the release of 1.17.0, |
|
getting a strange error during checks regarding the |
|
Don't worry about that one, it is a heisenbug, nobody quite knows where it comes from... I suspsect a heisenbug within OpenBLAS or so (maybe in the thread scheduling, but the last time I looked I saw things that were not real). |
It seemed to pop up after I used the Maybe it is a bug in pytest? |
|
No, the |
hameerabbasi
left a comment
hameerabbasi
left a comment
There was a problem hiding this comment.
Some general notes on correctness.
| if method == 'cholesky': | ||
| x = np.dot(x, l) | ||
| else: | ||
| x = np.dot(x, u * np.sqrt(s)) |
There was a problem hiding this comment.
Same applies here, should use np.conj(u.T).
…ctor separately for each method
…eigh Co-Authored-By: Eric Wieser <[email protected]>
|
There is a merge conflict that needs to be resolved before merging |
I think the |
|
Either ought to work fine |
|
One of the Travis CI builds failed on python 3.6 with environment variables |
|
travis run on ppc is failing since cython outputs a warning: |
I also now get this error while building numpy to run tests locally: |
|
what happens when you clean up your build environment |
|
The warning is coming from building a new cython version 0.29.14 from the source package. It seems the CI may have run during a package upload, when the binary wheels were not available. |
|
Thanks @zoj613 and @eric-wieser for the review. |
It runs just fine. I already tried it twice before posting the error here. Here is the output: |
|
It ran fine for me locally |
This PR addresses issue # #14193.
As per discussion had with @charris and @bashtage, I replaced the current slow method using SVD for factorization with the faster eigen decomposition method. The SVD method can still be accessible for legacy reasons via the keyword argument
mode='legacy'. A much faster but less robust method using Cholesky factorization can be used viamode='fast'. I also added the option for the user to explicitely provide the factor matrix so it doesn't get recomputed in situations that require repeated calls to the function.