-
Notifications
You must be signed in to change notification settings - Fork 48
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
Add sparseL extractor and use it in condVar #492
Conversation
Co-authored-by: Phillip Alday <[email protected]>
Codecov Report
@@ Coverage Diff @@
## main #492 +/- ##
==========================================
+ Coverage 94.14% 94.25% +0.10%
==========================================
Files 26 26
Lines 2239 2296 +57
==========================================
+ Hits 2108 2164 +56
- Misses 131 132 +1
Continue to review full report at Codecov.
|
For testing on v"1.7.0-DEV" I tried to wrap any fitting of GLMMs in conditionals on VERSION but it turned into a game of whack-a-mole. I did check that the tests on |
I decided to play whack-a-mole and found a case which went into the infinite inference loop without trying to fit a GLMM. julia> using MixedModels, Random
julia> fm = fit(MixedModel, @formula(yield ~ 1 + (1|batch)), MixedModels.dataset(:dyestuff));
julia> bsamp = parametricbootstrap(MersenneTwister(1234321), 100, fm); At this point I was able to trace it back a little further. |
If |
Should we add something analogous to |
Yes, please --- and in this context: Markdown output for CPs would be nice, too. |
Okay, I will add a function like |
Another enhancement I was thinking of is to specify which grouping factors you want the |
In a function like |
Standard deviations and correlations would be my preference -- that's also what people are going to care about for making dot plots. Power users who need the covariance matrices should be able to deal with the less user friendly
Type stability and simplicity of interface would be my argument for only doing one. 😄 |
I agree with Phillip. Sometimes life is easier than you think. |
I was going to add more to this PR but I think it is best to get it into main now and worry about non-breaking enhancements later. |
@palday Could you check on the conflict in |
@@ -25,7 +25,7 @@ end | |||
|
|||
Return the average of `a` and `b` | |||
""" | |||
average(a::T, b::T) where {T<:AbstractFloat} = (a + b) / 2 | |||
average(a::T, b::T) where {T<:AbstractFloat} = (a + b) / T(2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you run into trouble with this somewhere? As an integer 2
should be promoted to whatever floating point type (a+b)
is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought there would be an issue with Float32
in that the literal 2
is an Int
which will get promoted to Float64
on a 64-bit system. But I was wrong
julia> (1.0f0 + 2.0f0) / 2
1.5f0
so can go back to the original expression.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought I would leave that in as it doesn't seem to be harmful in any way and it could be protection against unforeseen circumstances.
* Add sparseL extractor and use it in condVar * Add and export condVarTables. Comment out failing tests. * update version bound for condVar * make cvtbl internal * remove some unused type restrictions * Remove guards on code for v1.7.0-DEV * Add tests of convVartable Co-authored-by: Phillip Alday <[email protected]> (cherry picked from commit cbcc1e1)
* Remove DataFramesMeta from the docs (#515) * fix reflink * remove DataFramesMeta from the docs (cherry picked from commit 628cbf3) * CompatHelper: bump compat for "Distributions" to "0.25" (#516) Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> (cherry picked from commit 16bb7fb) * Add sparseL extractor and use it in condVar (#492) * Add sparseL extractor and use it in condVar * Add and export condVarTables. Comment out failing tests. * update version bound for condVar * make cvtbl internal * remove some unused type restrictions * Remove guards on code for v1.7.0-DEV * Add tests of convVartable Co-authored-by: Phillip Alday <[email protected]> (cherry picked from commit cbcc1e1) * omit lowerbound on beta for GLMM bootstrap (#518) * news update (cherry picked from commit a3c33de) * NEWS links * minor version bump Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Douglas Bates <[email protected]>
sparseL
extracts theL
matrix as a lower-triangularSparseMatrixCSC
condVar
to solve the lower-triangular system usingL
and a block of columns fromsΛ'
, The X'X product of this solution is a block of thecondVar
array.SparseArrays
does not handle integer types other thanInt
correctly. It's a trivial fix but still needs to be done.