AbstractGPs.jl icon indicating copy to clipboard operation
AbstractGPs.jl copied to clipboard

Use non scalar method to compute logdet for cholesky and relax compat for PDMat

Open sharanry opened this issue 2 years ago • 6 comments

This enables the computation of dtc on GPU

sharanry avatar Jul 21 '22 12:07 sharanry

Will be creating an issue in https://github.com/JuliaLang/julia

sharanry avatar Jul 21 '22 12:07 sharanry

Could you add a test to make sure that logdet_ == logdet ?

theogf avatar Jul 21 '22 13:07 theogf

Also why the relaxation on the PDMats Compat?

theogf avatar Jul 21 '22 13:07 theogf

Also why the relaxation on the PDMats Compat?

I encounter this without the relaxation

(WilliamsGPR) pkg> dev AbstractGPs
   Resolving package versions...
ERROR: Unsatisfiable requirements detected for package ElasticPDMats [2904ab23]:
 ElasticPDMats [2904ab23] log:
 ├─possible versions are: 0.1.0-0.2.2 or uninstalled
 ├─restricted by compatibility requirements with GaussianProcesses [891a1506] to versions: 0.2.0-0.2.2
 │ └─GaussianProcesses [891a1506] log:
 │   ├─possible versions are: 0.8.0-0.12.4 or uninstalled
 │   └─restricted to versions 0.12 by WilliamsGPR [c957e16a], leaving only versions 0.12.0-0.12.4
 │     └─WilliamsGPR [c957e16a] log:
 │       ├─possible versions are: 0.2.0 or uninstalled
 │       └─WilliamsGPR [c957e16a] is fixed to version 0.2.0
 └─restricted by compatibility requirements with PDMats [90014a1f] to versions: uninstalled — no versions left
   └─PDMats [90014a1f] log:
     ├─possible versions are: 0.9.0-0.11.16 or uninstalled
     └─restricted to versions 0.11 by AbstractGPs [99985d1d], leaving only versions 0.11.0-0.11.16
       └─AbstractGPs [99985d1d] log:
         ├─possible versions are: 0.5.14 or uninstalled
         ├─restricted to versions 0.5 by WilliamsGPR [c957e16a], leaving only versions 0.5.14
         │ └─WilliamsGPR [c957e16a] log: see above
         └─AbstractGPs [99985d1d] is fixed to version 0.5.14

sharanry avatar Jul 21 '22 16:07 sharanry

Codecov Report

Merging #329 (8aae82a) into master (6ee8549) will increase coverage by 0.00%. The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #329   +/-   ##
=======================================
  Coverage   97.62%   97.63%           
=======================================
  Files          10       10           
  Lines         379      380    +1     
=======================================
+ Hits          370      371    +1     
  Misses          9        9           
Impacted Files Coverage Δ
src/sparse_approximations.jl 100.00% <100.00%> (ø)

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 6ee8549...8aae82a. Read the comment docs.

codecov[bot] avatar Jul 21 '22 16:07 codecov[bot]

I don't understand the changes in this PR, can you explain the motivation?

The compat issues seem to be caused by ElasticPDMats/GaussianProcesses/WilliamsGPR. I don't remember the details but there was surely a reason for why we dropped support for PDMata 0.10. So I think it would be better to fix the other problematic packages but not to reintroduce the PDMats 0.10 compat entry.

The change to _logdet is also unclear to me as it breaks the package in its current form. I guess you maybe forgot to define _logdet(x) = logdet(x) fallback? In any case, I don't think such an internal method for Cholesky should be introduced here. The logdet definition for Cholesky in base (https://github.com/JuliaLang/julia/blob/46a6f2230d9d05f0bb26ca5d241a7316993c10a8/stdlib/LinearAlgebra/src/cholesky.jl#L669) is fine, and I assume GPU support should be added in CUDA.jl or similar packages, probably by defining something like logdet(::Cholesky{T,CuMatrix{T}}) (see eg also https://github.com/JuliaGPU/CUDA.jl/issues/110).

devmotion avatar Jul 21 '22 19:07 devmotion

I'll close this PR for now. Please reopen it if you intend to address https://github.com/JuliaGaussianProcesses/AbstractGPs.jl/pull/329#issuecomment-1191856948 :slightly_smiling_face:

devmotion avatar Sep 26 '22 18:09 devmotion