Skip to content
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

Merged
merged 10 commits into from
May 8, 2021
Merged

Add sparseL extractor and use it in condVar #492

merged 10 commits into from
May 8, 2021

Conversation

dmbates
Copy link
Collaborator

@dmbates dmbates commented Mar 23, 2021

  • sparseL extracts the L matrix as a lower-triangular SparseMatrixCSC
  • rewrite condVar to solve the lower-triangular system using L and a block of columns from sΛ', The X'X product of this solution is a block of the condVar array.
  • needs tests but currently fails b/c the lower-triangular solution in SparseArrays does not handle integer types other than Int correctly. It's a trivial fix but still needs to be done.

@dmbates dmbates marked this pull request as draft March 23, 2021 22:47
Base automatically changed from master to main March 24, 2021 19:52
src/linearmixedmodel.jl Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Mar 28, 2021

Codecov Report

Merging #492 (ec187d0) into main (16bb7fb) will increase coverage by 0.10%.
The diff coverage is 98.48%.

Impacted file tree graph

@@            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     
Impacted Files Coverage Δ
src/linearmixedmodel.jl 95.71% <98.24%> (+0.23%) ⬆️
src/utilities.jl 96.36% <100.00%> (+0.61%) ⬆️

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 16bb7fb...ec187d0. Read the comment docs.

@dmbates
Copy link
Collaborator Author

dmbates commented Mar 28, 2021

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 condVar in pls.jl succeeded.

@dmbates
Copy link
Collaborator Author

dmbates commented Mar 28, 2021

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 bsamp.objective causes the infinite loop for me. Could others check it out under v"1.7.0-DEV.760" or later?

I was able to trace it back a little further. typeof(bsamp.fits) also causes an infinite loop.

@palday
Copy link
Member

palday commented Mar 29, 2021

If typeof(bsamp.fits) is causing an infinite loop, I don't think there's much to do except keep bumping the relevant Julia issue. If the pls.jl tests pass locally for you on 1.7-DEV, I'm fine with that and we can revisit the problem when the upstream bug is fixed.

@palday
Copy link
Member

palday commented Mar 29, 2021

Should we add something analogous to raneftables for condVar?

@kliegl
Copy link

kliegl commented Mar 29, 2021

Yes, please --- and in this context: Markdown output for CPs would be nice, too.

@dmbates
Copy link
Collaborator Author

dmbates commented Mar 29, 2021

Okay, I will add a function like raneftables for condVar later today.

@dmbates
Copy link
Collaborator Author

dmbates commented Mar 29, 2021

Another enhancement I was thinking of is to specify which grouping factors you want the condVar array for. The point is that it is much easier to evaluate them for the grouping factors with fewer random effects associated with each level. I think @kliegl mentioned that he had hundreds of thousands of students but only hundreds of schools and doesn't need the conditional variances for the students. They will take much longer to evaluate that will the conditional variances for the schools and districts.

@dmbates
Copy link
Collaborator Author

dmbates commented Mar 29, 2021

In a function like raneftables for the conditional variances, how do you want to handle the covariance matrices? Should it just return a table of covariance matrices or should it split them into standard deviations and correlations? (Why do I think the answer is going to be, "why not both?"?)

@palday
Copy link
Member

palday commented Mar 29, 2021

In a function like raneftables for the conditional variances, how do you want to handle the covariance matrices? Should it just return a table of covariance matrices or should it split them into standard deviations and correlations?

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 condVar directly.

(Why do I think the answer is going to be, "why not both?"?)

Type stability and simplicity of interface would be my argument for only doing one. 😄

@kliegl
Copy link

kliegl commented Mar 29, 2021

I agree with Phillip. Sometimes life is easier than you think.

@dmbates dmbates marked this pull request as ready for review May 5, 2021 19:40
@dmbates
Copy link
Collaborator Author

dmbates commented May 5, 2021

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.

@dmbates
Copy link
Collaborator Author

dmbates commented May 5, 2021

@palday Could you check on the conflict in test/mime.jl?

@@ -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)
Copy link
Member

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.

Copy link
Collaborator Author

@dmbates dmbates May 7, 2021

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.

Copy link
Collaborator Author

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.

test/bootstrap.jl Outdated Show resolved Hide resolved
test/pls.jl Show resolved Hide resolved
@dmbates dmbates merged commit cbcc1e1 into main May 8, 2021
@dmbates dmbates deleted the condVar branch May 8, 2021 15:04
palday pushed a commit that referenced this pull request May 10, 2021
* 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)
palday added a commit that referenced this pull request May 10, 2021
* 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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants