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

RFC: Remove PDMats dependency? #940

Closed
nickrobinson251 opened this issue Jul 23, 2019 · 10 comments
Closed

RFC: Remove PDMats dependency? #940

nickrobinson251 opened this issue Jul 23, 2019 · 10 comments

Comments

@nickrobinson251
Copy link
Contributor

No description provided.

@ararslan
Copy link
Member

Can you elaborate on why this would be beneficial?

@matbesancon
Copy link
Member

this has been discussed in other issues, having PDMats.jl brings in very heavy dependencies, it would be better to use AbstractPDMat which would ideally live in a package separated from the PDMat implementation

@matbesancon
Copy link
Member

@bethandtownes
Copy link

I agree that it's better without PDMat since ARPACK is not well supported on Julia built with Intel MKL library. Plus there are some other well written ARPACK alternatives that performs just as well, e.g. KrylovKit.jl and IterativeSolvers.jl.

@vilim
Copy link

vilim commented Mar 22, 2020

Could this please be considered again? Distributions is the only package I commonly use and that fails with MKL
I noticed there is also ArpacMKL, but I don't know how to link it to PDMat

@iamed2
Copy link

iamed2 commented May 22, 2020

PDMats no longer requires Arpack

@matbesancon
Copy link
Member

@nickrobinson251 closing this, feel free to chime in if there are still motivation for that

@johnczito
Copy link
Member

I encountered another motivation for this. The Turing folks have a package DistributionsAD.jl which, among other things, includes line-for-line recodings of the MvNormal, Dirichlet, Wishart, InverseWishart, and more. One of the reasons they find this necessary is because PDMats apparently doesn't play nice with the AD stuff they're trying to do (see here, and surrounding chat).

It would definitely be better if they could just build on top of what's provided here.

@ararslan
Copy link
Member

ararslan commented Jul 6, 2020

From that issue:

PDMats also causes issues when trying to differentiate wrt the random variable for almost all AD backends

Seems like that's something that should be addressed in PDMats rather than just removing PDMats from other packages. If it breaks AD systems because of the Cholesky decomposition, that's differentiable (and is implemented in Nabla and ChainRules) so it should be fixable. Otherwise it would be good if AD folks could open issues on PDMats.

@johnczito
Copy link
Member

Seems like that's something that should be addressed in PDMats rather than just removing PDMats from other packages.

Good point. And using ChainRules to fix this has come up.

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

No branches or pull requests

7 participants