Diagonalize ZeroCorr

[palday]

Closes #487 .

I had to loosen some type constraints in various places to make this work and I didn't add in any additional mul! methods, so I don't know if we're fully taking advantages of this. @dmbates Feel free to add those in if you want, but those wouldn't be breaking changes.

I also changed rownormalize! to be non mutating. This made specializing for Diagonal easier and actually cut out an extra allocation.

[codecov]

Codecov Report

Merging #489 (a77a08a) into master (9fc9e50) will decrease coverage by 0.06%.
The diff coverage is 92.30%.

❗ Current head a77a08a differs from pull request most recent head 9a90522. Consider uploading reports for the commit 9a90522 to get more accurate results

@@            Coverage Diff             @@
##           master     #489      +/-   ##
==========================================
- Coverage   94.01%   93.95%   -0.07%     
==========================================
  Files          27       27              
  Lines        2140     2151      +11     
==========================================
+ Hits         2012     2021       +9     
- Misses        128      130       +2     

Impacted Files	Coverage Δ
src/utilities.jl	95.45% <50.00%> (-4.55%)	⬇️
src/pca.jl	98.07% <100.00%> (ø)
src/remat.jl	96.28% <100.00%> (+0.09%)	⬆️

---

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 9fc9e50...9a90522. Read the comment docs.

[dmbates]

Thanks. Taking a look now.

[dmbates]

It is not great that I am just remembering this but I think that taking advantage of this change in the multiplications will either involve type piracy or a PR on the LinearAlgebra package. The reason is that methods like

function lmulΛ!(adjA::Adjoint{T,ReMat{T,S}}, B::VecOrMat{T}) where {T,S}
    lmul!(adjoint(adjA.parent.λ), reshape(B, S, :))
    B
end

get to the point of calling lmul! before the type of λ is known. So we would have to override methods of lmul!.

In retrospect I think the best approach is to define the type of λ to be Union{Diagonal{T,Vector{T}}, LowerTriangular{T,Matrix{T}}}. I make those modifications if you agree @palday

[palday]

In retrospect I think the best approach is to define the type of λ to be Union{Diagonal{T,Vector{T}}, LowerTriangular{T,Matrix{T}}}. I make those modifications if you agree @palday

If we ever implement other covariance structures, are there special matrix forms that occur there? In that case, I would make λ to be <:AbstractMatrix{T} and then remove the LowerTriangular wrapper around the Diagonal things I added (so that we could add in specialization for those structures without needing additional change to the ReMat type). If not, then your approach is simpler.

Go with whatever you think is best!

[dmbates]

@palday For the time being I think I will stay with Union{LowerTriangular{T,Matrix{T}},Diagonal{T,Vector{T}}} for the type. I believe that the compiler can handle small unions better than more general supertypes.

How would I go about turning off the Future.yml spec for the CI runs? Right now it needs to be stopped manually.

I have a version that passes its tests but I want to be able to turn off the Future CI before pushing it.

[palday]

I've disabled Future and I think you're right on the compiler stuff.

[palday]

It's my own pull request, so I can't approve it, but it looks good to me!

[dmbates]

@palday Would you take another look at this and merge if it looks okay to you?