Implement fallbacks for (un)whiten(!) and (inv)quad(!) to AbstractArray #162

oxinabox · 2022-05-19T12:36:22Z

This is the partner to JuliaStats/Distributions.jl#1552 (comment)
which will allow resolving JuliaStats/Distributions.jl#1219

It doesn't go and implement everything falling back to AbstractArray,
just the operations required for Multivariate Normals,
and their family (so I can test them)

In some cases that implementation is just making the PDMat one generic, (since cholesky(x::PDMat)=x.chol is defined, etc.

src/pdmat.jl

src/utils.jl

src/generics.jl

test/runtests.jl

Co-authored-by: David Widmann <devmotion@users.noreply.github.com>

devmotion

Looks good to me in principle, but I would like to get someone else's opinion (@andreasnoack maybe?).

When commenting on the function signatures in the docstrings, I noticed that the general definitions for AbstractMatrix could cause surprising slow-downs for AbstractPDMats with non-StridedArrays if their specializations for (inv)quad! etc. are only defined for StridedArrays. So maybe it would be good to check if such restrictions still exist and if we can remove them.

README.md

devmotion · 2022-05-20T06:11:30Z

src/generics.jl


 """
+    whiten(a::AbstractMatrix, x::AbstractVecOrMat)    
    whiten(a::AbstractPDMat, x::StridedVecOrMat)


AbstractPDMat <: AbstractMatrix, so maybe these signatures can be removed?

src/generics.jl

Co-authored-by: David Widmann <devmotion@users.noreply.github.com>

codecov-commenter · 2022-05-20T13:03:07Z

Codecov Report

Merging #162 (334ca80) into master (92a5178) will decrease coverage by 0.14%.
The diff coverage is 92.00%.

@@            Coverage Diff             @@
##           master     #162      +/-   ##
==========================================
- Coverage   90.33%   90.19%   -0.15%     
==========================================
  Files           8        8              
  Lines         414      418       +4     
==========================================
+ Hits          374      377       +3     
- Misses         40       41       +1

Impacted Files	Coverage Δ
src/pdmat.jl	`97.87% <ø> (-0.38%)`	⬇️
src/utils.jl	`91.66% <80.00%> (ø)`
src/generics.jl	`87.50% <95.00%> (+2.88%)`	⬆️

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 92a5178...334ca80. Read the comment docs.

oxinabox · 2022-05-20T14:17:40Z

Looks good to me in principle, but I would like to get someone else's opinion (@andreasnoack maybe?).

When commenting on the function signatures in the docstrings, I noticed that the general definitions for AbstractMatrix could cause surprising slow-downs for AbstractPDMats with non-StridedArrays if their specializations for (inv)quad! etc. are only defined for StridedArrays. So maybe it would be good to check if such restrictions still exist and if we can remove them.

AFAIK those don't slow down.
They were MethodErrors before and now they are AmbiguityErrors

Co-authored-by: David Widmann <devmotion@users.noreply.github.com>

devmotion · 2022-05-20T21:41:31Z

Are you sure? To me it seems e.g. that

PDMats.jl/src/pdiagmat.jl

Line 113 in 92a5178

function quad!(r::AbstractArray, a::PDiagMat, x::StridedMatrix)

defines an optimization of quad! for a::PDiagMat and x::StridedMatrix but by adding the generic fallback in this PR a call of e.g. quad!(r::AbstractArray, a::PDiagMat, x::SubArray{Float64,2}) will not use this specialized implementation but the less efficient fallback.

oxinabox · 2022-05-24T13:10:40Z

Looks fine to me:

With this PR:

julia> @which invquad!([2,2], PDiagMat([1,1]), [3 3; 3 3])
invquad!(r::AbstractArray, a::PDiagMat, x::StridedMatrix{T} where T) in PDMats at /home/oxinabox/JuliaEnvs/Distributions.jl/dev/PDMats/src/pdiagmat.jl:127

julia> @which invquad!([2,2], PDiagMat([1,1]), @view([3 3; 3 3][:,:]))
invquad!(r::AbstractArray, a::PDiagMat, x::StridedMatrix{T} where T) in PDMats at /home/oxinabox/JuliaEnvs/Distributions.jl/dev/PDMats/src/pdiagmat.jl:127

julia> @which invquad!([2,2], PDiagMat([1,1]), @view([3 3; 3 3][[2 4; 1 3]]))
invquad!(r::AbstractArray, a::AbstractMatrix, x::AbstractMatrix) in PDMats at /home/oxinabox/JuliaEnvs/Distributions.jl/dev/PDMats/src/generics.jl:140

Without this PR:

julia> @which invquad!([2,2], PDiagMat([1,1]), [3 3; 3 3])
invquad!(r::AbstractArray, a::PDiagMat, x::StridedMatrix{T} where T) in PDMats at /home/oxinabox/.julia/packages/PDMats/pIpaj/src/pdiagmat.jl:127

julia> @which invquad!([2,2], PDiagMat([1,1]), @view([3 3; 3 3][:,:]))
invquad!(r::AbstractArray, a::PDiagMat, x::StridedMatrix{T} where T) in PDMats at /home/oxinabox/.julia/packages/PDMats/pIpaj/src/pdiagmat.jl:127

julia> @which invquad!([2,2], PDiagMat([1,1]), @view([3 3; 3 3][[2 4; 1 3]]))
ERROR: no unique matching method found for the specified argument types

The only difference I see for this case is that the last one is now longer an error

devmotion · 2022-05-24T13:47:02Z

The only difference I see for this case is that the last one is now longer an error

Yes, exactly, that's what I meant - it's not an error anymore but it uses the generic fallback instead of the method for diagonal matrices.

oxinabox · 2022-05-24T14:03:25Z

ah right, there is a lot of code here that I think only works for StridedVectors, or rather code that only really works for things indexed by 1:length(x).
I think easiest would be to go through and generalize every single method in the package.
Which I am happy to make a follow up PR to do, but i think it would make this PR to large.
I will start that PR now, branching off this one.

src/utils.jl

oxinabox · 2022-05-25T10:11:38Z

Given #163 is open, how do you feel about merging this?
The only behavourally change is to things that errored before.
And we have PRs open that will also make some of those fast.

src/utils.jl

devmotion

Looks good to me, thanks!

Any additional comments or thoughts, @andreasnoack?

matbesancon · 2022-05-28T17:58:09Z

Any idea where the nightly tests error comes from?
https://github.com/JuliaStats/PDMats.jl/runs/6593682307?check_suite_focus=true#step:6:185

BandedMatrices: Error During Test at /home/runner/work/PDMats.jl/PDMats.jl/test/specialarrays.jl:50
  Got exception outside of a @test
  UndefVarError: liblapack not defined
  Stacktrace:
    [1] pbtrf!(uplo::Char, m::Int64, kd::Int64, A::Matrix{Float64})
      @ BandedMatrices ~/.julia/packages/BandedMatrices/dt29n/src/lapack.jl:210
    [2] banded_chol!
      @ ~/.julia/packages/BandedMatrices/dt29n/src/symbanded/BandedCholesky.jl:4 [inlined]

devmotion · 2022-05-28T19:34:55Z

Seems to be caused by the tests with BandedMatrices, so it might not be related to (or rather caused by) PDMats.

devmotion · 2022-05-28T19:36:58Z

The same test errors we've seen for a while on the master branch (eg https://github.com/JuliaStats/PDMats.jl/runs/6388016866?check_suite_focus=true), so it does not seem related to this PR.

oxinabox · 2022-05-31T15:52:02Z

can this please be merged?
this is blocking some work i want to wrap up

matbesancon · 2022-05-31T16:00:03Z

done, thanks!

oxinabox added 3 commits May 19, 2022 13:28

Test AbstractArray fallbacks

be2a97a

Implement whiten and quad and co AbstractArray fallbacks

818c329

Bump version

5baf62d

oxinabox commented May 19, 2022

View reviewed changes

src/pdmat.jl Show resolved Hide resolved

oxinabox commented May 19, 2022

View reviewed changes

src/utils.jl Show resolved Hide resolved

devmotion reviewed May 19, 2022

View reviewed changes

src/generics.jl Show resolved Hide resolved

devmotion reviewed May 19, 2022

View reviewed changes

test/runtests.jl Outdated Show resolved Hide resolved

Update test/runtests.jl

3a87d90

Co-authored-by: David Widmann <devmotion@users.noreply.github.com>

oxinabox requested a review from devmotion May 19, 2022 14:06

oxinabox mentioned this pull request May 19, 2022

Remove constraint on AbstractPDMat JuliaStats/Distributions.jl#1552

Open

6 tasks

devmotion reviewed May 20, 2022

View reviewed changes

Update README.md

7efd1f3

Co-authored-by: David Widmann <devmotion@users.noreply.github.com>

Apply suggestions from code review

c0cd474

Co-authored-by: David Widmann <devmotion@users.noreply.github.com>

Remove duplicated method

37c7eaf

clean out redundant docstrings

91e47ef

oxinabox commented May 24, 2022

View reviewed changes

src/utils.jl Outdated Show resolved Hide resolved

check axes match in colwise_dot!

59c68b0

oxinabox mentioned this pull request May 24, 2022

Remove StridedMatrix restriction and use modern index handling #163

Merged

devmotion reviewed May 25, 2022

View reviewed changes

src/utils.jl Outdated Show resolved Hide resolved

oxinabox commented May 25, 2022

View reviewed changes

src/utils.jl Show resolved Hide resolved

also check j is valid index for r

7c5b464

oxinabox commented May 25, 2022

View reviewed changes

src/utils.jl Show resolved Hide resolved

both checks in colwisedot

334ca80

devmotion approved these changes May 25, 2022

View reviewed changes

matbesancon merged commit 6674103 into JuliaStats:master May 31, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement fallbacks for (un)whiten(!) and (inv)quad(!) to AbstractArray #162

Implement fallbacks for (un)whiten(!) and (inv)quad(!) to AbstractArray #162

oxinabox commented May 19, 2022

devmotion left a comment

devmotion May 20, 2022

codecov-commenter commented May 20, 2022 •

edited

Loading

oxinabox commented May 20, 2022

devmotion commented May 20, 2022

oxinabox commented May 24, 2022 •

edited

Loading

devmotion commented May 24, 2022

oxinabox commented May 24, 2022

oxinabox commented May 25, 2022

devmotion left a comment

matbesancon commented May 28, 2022

devmotion commented May 28, 2022

devmotion commented May 28, 2022

oxinabox commented May 31, 2022 •

edited

Loading

matbesancon commented May 31, 2022

Implement fallbacks for (un)whiten(!) and (inv)quad(!) to AbstractArray #162

Implement fallbacks for (un)whiten(!) and (inv)quad(!) to AbstractArray #162

Conversation

oxinabox commented May 19, 2022

devmotion left a comment

Choose a reason for hiding this comment

devmotion May 20, 2022

Choose a reason for hiding this comment

codecov-commenter commented May 20, 2022 • edited Loading

Codecov Report

oxinabox commented May 20, 2022

devmotion commented May 20, 2022

oxinabox commented May 24, 2022 • edited Loading

With this PR:

Without this PR:

devmotion commented May 24, 2022

oxinabox commented May 24, 2022

oxinabox commented May 25, 2022

devmotion left a comment

Choose a reason for hiding this comment

matbesancon commented May 28, 2022

devmotion commented May 28, 2022

devmotion commented May 28, 2022

oxinabox commented May 31, 2022 • edited Loading

matbesancon commented May 31, 2022

codecov-commenter commented May 20, 2022 •

edited

Loading

oxinabox commented May 24, 2022 •

edited

Loading

oxinabox commented May 31, 2022 •

edited

Loading