Rank update #331

dmbates · 2020-06-25T21:24:11Z

Clean up rankUpdate! methods. In particular use Hermitian{T, Diagonal{T, Vector{T}}} instead of branching on the diagonal case in updateL!. We want to use method dispatch instead of if statements that I had in the code. This should be rebased on master after #329 is merged.

Closes #290.

codecov · 2020-06-25T21:34:07Z

Codecov Report

Merging #331 into master will decrease coverage by 0.15%.
The diff coverage is 86.20%.

@@            Coverage Diff             @@
##           master     #331      +/-   ##
==========================================
- Coverage   95.91%   95.75%   -0.16%     
==========================================
  Files          21       22       +1     
  Lines        1492     1485       -7     
==========================================
- Hits         1431     1422       -9     
- Misses         61       63       +2

Impacted Files	Coverage Δ
src/arraytypes.jl	`95.12% <0.00%> (-4.88%)`	⬇️
src/remat.jl	`95.00% <86.66%> (+0.05%)`	⬆️
src/linalg/rankUpdate.jl	`97.67% <100.00%> (-0.77%)`	⬇️
src/linearmixedmodel.jl	`100.00% <100.00%> (ø)`
src/randomeffectsterm.jl	`90.19% <0.00%> (ø)`
src/generalizedlinearmixedmodel.jl	`83.33% <0.00%> (ø)`
src/schema.jl	`100.00% <0.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 64ba631...0f1c114. Read the comment docs.

palday

If I understand everything correctly, then it looks pretty good. I have one question about assignment vs. dot assignment in a single method that needs attention, everything else is small.

The two big changes that I didn't comment on are:

No default values for alpha and beta in the rank update methods. I think this is fine, but this is definitely a breaking change (admittedly on non-exported methods), so it's a good thing to get in now before the big release. Is this also in line with the related methods in LinearAlgebra?
Removal of a catch-all rankUpdate! method. I'm wondering if we should still have that in there. Or maybe even something 'dumber', such as

function rankUpdate(A::Any, B::Any, alpha, beta) 
   error("We haven't implemented a method for $(typeof(A)), $(typeof(B)). Please file an issue on GitHub")
end

src/linalg/rankUpdate.jl

palday · 2020-07-07T20:09:42Z

src/linalg/rankUpdate.jl

    end
    C
 end
+=#


suggest final newline so that various things stop complaining

Suggested change

=#

=#

palday · 2020-07-07T20:11:07Z

src/linearmixedmodel.jl

@@ -531,7 +531,7 @@ end

 nθ(m::LinearMixedModel) = length(m.parmap)

-StatsBase.nobs(m::LinearMixedModel) = first(size(m))
+StatsBase.nobs(m::LinearMixedModel) = length(first(m.allterms).refs)


why change this? (not opposed, just curious)

src/remat.jl

palday · 2020-07-07T20:17:04Z

test/linalg.jl

@@ -52,6 +52,7 @@ end
    @test loglikelihood(fit!(wm1)) ≈ loglikelihood(m1)
 end

+#=  I don't see this testset as meaningful b/c diagonal A does not occur after amalgamation of ReMat's for the same grouping factor - D.B.


I'm trying to think of a case where diagonal A could occur. Perhaps in one of the custom model types some of the ZiF spinoff projects are working on? Maybe this is something re-visit in a few weeks to months and see if we can further trim.

It is best to think of how rankUpdate! gets called. After pre- and post-multiplication by Lambda and inflation of the diagonal the rankUpdate! methods get called with C being one of the diagonal blocks and A being a block to the left of C. So C is m.L[Block(2,2)] or higher because there are no blocks to the left of m.L[Block(1,1)]. The cases that matter are when there is more than one grouping factor for the random effects. If A were to be diagonal then it would need to be generated from two scalar random-effects terms, as in (1|G) + (1|H) and the number of levels of G and H would need to be equal and the levels would match. You can get that from (1|G) + (0+x|G) but that is now collapsed to a single r.e. term either through zerocorr(1+x|G) or by amalgamation of the random effects terms. So I don't think there will be real-life cases where A is diagonal. C is always a square matrix and either Diagonal or UniformBlockDiagonal or, through fill-in, Dense. The only special cases for A are Dense, SparseMatrixCSC or, in a new branch that I haven't pushed yet, KHotColumn. The last one is a special type of sparse matrix generated by nested grouping factors for vector-valued random effects where each column of A has exactly K adjacent non-zeros.

Right, so the only way that this could occur would be if someone had two identical grouping variables so that they wouldn't be amalgamated. Until we see a legitimate use case for that, I think we can safely ignore that possibility.

Thanks for the refresher! 😄

src/linalg/rankUpdate.jl

dmbates · 2020-07-10T21:51:14Z

If I understand everything correctly, then it looks pretty good. I have one question about assignment vs. dot assignment in a single method that needs attention, everything else is small.

The two big changes that I didn't comment on are:

1. No default values for alpha and beta in the rank update methods. I think this is fine, but this is definitely a breaking change (admittedly on non-exported methods), so it's a good thing to get in now before the big release. Is this also in line with the related methods in LinearAlgebra?

2. Removal of a catch-all `rankUpdate!` method. I'm wondering if we should still have that in there. Or maybe even something 'dumber', such as

function rankUpdate(A::Any, B::Any, alpha, beta) 
   error("We haven't implemented a method for $(typeof(A)), $(typeof(B)). Please file an issue on GitHub")
end

I don't see the benefit of adding a "catch-all" method to generate an error. Not having a method has the same effect, doesn't it?

dmbates · 2020-07-10T21:55:28Z

If I understand everything correctly, then it looks pretty good. I have one question about assignment vs. dot assignment in a single method that needs attention, everything else is small.

The two big changes that I didn't comment on are:
1. No default values for alpha and beta in the rank update methods. I think this is fine, but this is definitely a breaking change (admittedly on non-exported methods), so it's a good thing to get in now before the big release. Is this also in line with the related methods in LinearAlgebra?

I felt that this generic would be internal to the package and I dropped the default values for alpha and beta because there is only one call to generic in the package. It seemed it was easier to specify the values there than to add defaults for all the methods.

palday · 2020-07-10T22:05:51Z

If I understand everything correctly, then it looks pretty good. I have one question about assignment vs. dot assignment in a single method that needs attention, everything else is small.
The two big changes that I didn't comment on are:
1. No default values for alpha and beta in the rank update methods. I think this is fine, but this is definitely a breaking change (admittedly on non-exported methods), so it's a good thing to get in now before the big release. Is this also in line with the related methods in LinearAlgebra?

2. Removal of a catch-all `rankUpdate!` method. I'm wondering if we should still have that in there. Or maybe even something 'dumber', such as
function rankUpdate(A::Any, B::Any, alpha, beta) 
   error("We haven't implemented a method for $(typeof(A)), $(typeof(B)). Please file an issue on GitHub")
end
I don't see the benefit of adding a "catch-all" method to generate an error. Not having a method has the same effect, doesn't it?

For us, yes. But for the average user, a MethodError followed by a stacktrace is big and scary and potentially looks like a bug. An error with "we haven't implemented this yet" seems less scary. But then again, both will generate huge stacktraces and that's probably the hardest thing for normal users anyway.

Ultimately, I'm neutral. Do we have a "normal" person we can ask?

dmbates · 2020-07-10T22:23:32Z

If I understand everything correctly, then it looks pretty good. I have one question about assignment vs. dot assignment in a single method that needs attention, everything else is small.
The two big changes that I didn't comment on are:
1. No default values for alpha and beta in the rank update methods. I think this is fine, but this is definitely a breaking change (admittedly on non-exported methods), so it's a good thing to get in now before the big release. Is this also in line with the related methods in LinearAlgebra?

2. Removal of a catch-all `rankUpdate!` method. I'm wondering if we should still have that in there. Or maybe even something 'dumber', such as
function rankUpdate(A::Any, B::Any, alpha, beta) 
   error("We haven't implemented a method for $(typeof(A)), $(typeof(B)). Please file an issue on GitHub")
end
I don't see the benefit of adding a "catch-all" method to generate an error. Not having a method has the same effect, doesn't it?
For us, yes. But for the average user, a MethodError followed by a stacktrace is big and scary and potentially looks like a bug. An error with "we haven't implemented this yet" seems less scary. But then again, both will generate huge stacktraces and that's probably the hardest thing for normal users anyway.

Ultimately, I'm neutral. Do we have a "normal" person we can ask?

I would value the opinions of either @christinabergmann or @debruine

dmbates · 2020-07-12T16:14:51Z

If I understand everything correctly, then it looks pretty good. I have one question about assignment vs. dot assignment in a single method that needs attention, everything else is small.
The two big changes that I didn't comment on are:
1. No default values for alpha and beta in the rank update methods. I think this is fine, but this is definitely a breaking change (admittedly on non-exported methods), so it's a good thing to get in now before the big release. Is this also in line with the related methods in LinearAlgebra?

2. Removal of a catch-all `rankUpdate!` method. I'm wondering if we should still have that in there. Or maybe even something 'dumber', such as
function rankUpdate(A::Any, B::Any, alpha, beta) 
   error("We haven't implemented a method for $(typeof(A)), $(typeof(B)). Please file an issue on GitHub")
end
I don't see the benefit of adding a "catch-all" method to generate an error. Not having a method has the same effect, doesn't it?
For us, yes. But for the average user, a MethodError followed by a stacktrace is big and scary and potentially looks like a bug. An error with "we haven't implemented this yet" seems less scary. But then again, both will generate huge stacktraces and that's probably the hardest thing for normal users anyway.

In retrospect I think it is fine to go ahead with adding a fallback-error method like this. Would you (@palday) be willing to add a test to ensure test coverage?

palday · 2020-07-13T09:53:41Z

@dmbates I'll take care of it. 😄

palday · 2020-07-14T11:02:07Z

test/linalg.jl

@@ -52,6 +52,7 @@ end
    @test loglikelihood(fit!(wm1)) ≈ loglikelihood(m1)
 end

+#=  I don't see this testset as meaningful b/c diagonal A does not occur after amalgamation of ReMat's for the same grouping factor - D.B.


Right, so the only way that this could occur would be if someone had two identical grouping variables so that they wouldn't be amalgamated. Until we see a legitimate use case for that, I think we can safely ignore that possibility.

Thanks for the refresher! 😄

palday · 2020-07-15T14:53:22Z

@dmbates If you're happy with the new error message, please squash and merge.

dmbates mentioned this pull request Jun 26, 2020

Add and use parmap field. #329

Merged

dmbates added 4 commits June 26, 2020 11:07

Avoid unnecessary calls to size(m)

4426bd4

first -> only for ReMat{T,1}

fab4acd

Clean up rankUpdate! and tests

9be0224

Hermitian{T,Diagonal{T}} must be more carefully spelled out

10de1e8

dmbates force-pushed the rankUpdate branch from fcd6feb to 10de1e8 Compare June 26, 2020 16:07

Tuning a couple of methods

52adccf

dmbates mentioned this pull request Jul 4, 2020

Add a dims field to LinearMixedModel #332

Merged

palday requested changes Jul 7, 2020

View reviewed changes

palday reviewed Jul 10, 2020

View reviewed changes

src/linalg/rankUpdate.jl Outdated Show resolved Hide resolved

dot assignment instead rebinding

9f3fb6a

generic method for rankUpdate!

0f1c114

palday approved these changes Jul 14, 2020

View reviewed changes

dmbates merged commit aef7ec7 into master Jul 15, 2020

palday deleted the rankUpdate branch November 19, 2020 17:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rank update #331

Rank update #331

dmbates commented Jun 25, 2020 •

edited by palday

Loading

codecov bot commented Jun 25, 2020 •

edited

Loading

palday left a comment

palday Jul 7, 2020

palday Jul 7, 2020

palday Jul 7, 2020

dmbates Jul 10, 2020

palday Jul 14, 2020

dmbates commented Jul 10, 2020

dmbates commented Jul 10, 2020

palday commented Jul 10, 2020

dmbates commented Jul 10, 2020

dmbates commented Jul 12, 2020

palday commented Jul 13, 2020

palday Jul 14, 2020

palday commented Jul 15, 2020

Rank update #331

Rank update #331

Conversation

dmbates commented Jun 25, 2020 • edited by palday Loading

codecov bot commented Jun 25, 2020 • edited Loading

Codecov Report

palday left a comment

Choose a reason for hiding this comment

palday Jul 7, 2020

Choose a reason for hiding this comment

palday Jul 7, 2020

Choose a reason for hiding this comment

palday Jul 7, 2020

Choose a reason for hiding this comment

dmbates Jul 10, 2020

Choose a reason for hiding this comment

palday Jul 14, 2020

Choose a reason for hiding this comment

dmbates commented Jul 10, 2020

dmbates commented Jul 10, 2020

palday commented Jul 10, 2020

dmbates commented Jul 10, 2020

dmbates commented Jul 12, 2020

palday commented Jul 13, 2020

palday Jul 14, 2020

Choose a reason for hiding this comment

palday commented Jul 15, 2020

dmbates commented Jun 25, 2020 •

edited by palday

Loading

codecov bot commented Jun 25, 2020 •

edited

Loading