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

Use 5-argument mul! instead of mulαβ! #231

Merged
merged 6 commits into from
Dec 23, 2019
Merged

Use 5-argument mul! instead of mulαβ! #231

merged 6 commits into from
Dec 23, 2019

Conversation

dmbates
Copy link
Collaborator

@dmbates dmbates commented Dec 10, 2019

Starting in Julia v1.3.0 methods for LinearAlgebra.mul! can be defined for 5 arguments, the last two being scalar multiples of A*B and of C. These are used in the blocked Cholesky factorization in the updateL! method for this package, which is the computational workhorse. Previous versions of this package defined a generic called mulαβ! for this but now that functionality can be expressed as methods for mul!.

* Require julia v1.3.0 or later
* Remove badge for coveralls
* Move `fit` methods from `mixed.jl` to other files
* Use 5-argument `mul!` methods
* Drop 2-stage optimization for GLMM fit with `fast=false`
@codecov-io
Copy link

codecov-io commented Dec 10, 2019

Codecov Report

Merging #231 into master will increase coverage by 1.75%.
The diff coverage is 88.35%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #231      +/-   ##
==========================================
+ Coverage   92.89%   94.65%   +1.75%     
==========================================
  Files          18       17       -1     
  Lines        1295     1234      -61     
==========================================
- Hits         1203     1168      -35     
+ Misses         92       66      -26
Impacted Files Coverage Δ
src/MixedModels.jl 100% <ø> (ø) ⬆️
src/femat.jl 100% <ø> (ø) ⬆️
src/linalg/logdet.jl 100% <100%> (ø) ⬆️
src/varcorr.jl 100% <100%> (ø) ⬆️
src/remat.jl 96.04% <100%> (-0.02%) ⬇️
src/simulate.jl 97.56% <100%> (+28.51%) ⬆️
src/gausshermite.jl 100% <100%> (ø) ⬆️
src/optsummary.jl 100% <100%> (ø) ⬆️
src/linalg/statschol.jl 100% <100%> (ø) ⬆️
src/arraytypes.jl 100% <100%> (ø) ⬆️
... and 8 more

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 3799c25...dbe9f57. Read the comment docs.

src/arraytypes.jl Show resolved Hide resolved
src/arraytypes.jl Show resolved Hide resolved
src/generalizedlinearmixedmodel.jl Show resolved Hide resolved
src/linalg/rankUpdate.jl Show resolved Hide resolved
src/linearmixedmodel.jl Show resolved Hide resolved
@@ -38,14 +38,36 @@ mutable struct OptSummary{T <: AbstractFloat}
feval::Int
optimizer::Symbol
returnvalue::Symbol
nAGQ::Integer # doesn't really belong here but I needed some place to store it
nAGQ::Integer # don't really belong here but I needed a place to store them
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would leave the singular verb and pronoun here.

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 meant to indicate that the two last values in the structure are not related to the optimization per se but are tucked away in here for lack of a better place to put them.

src/remat.jl Outdated Show resolved Hide resolved
src/varcorr.jl Show resolved Hide resolved
test/pirls.jl Show resolved Hide resolved
test/pirls.jl Show resolved Hide resolved
@palday
Copy link
Member

palday commented Dec 17, 2019

I think we're still missing tests for one-stage fast=false. I'll add them in.

Copy link
Collaborator Author

@dmbates dmbates left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are tests with fast=truein the "contra" and "grouseticks" test sets in test/pirls.jl. Did you want something in addition to those?

@palday
Copy link
Member

palday commented Dec 17, 2019

Something that would be problematic under the old two-stage method perhaps (I have some things from lexdec), i.e. the usual "show something didn't work but now does" test, although I'm not sure how much use that is here. The lexdec dataset will be part of the tests for gamma regression, which I've been tinkering with.

@palday
Copy link
Member

palday commented Dec 17, 2019

I've got the models for such a test sketched out in my fork and can move them over if you think testing the old boundary cases makes sense. Right now, MixedModels.jl and lme4 deliver somewhat different answers for the Bernoulli model and the correct answer for the Gamma model remains elusive.

@palday palday merged commit 705fb88 into master Dec 23, 2019
@dmbates dmbates deleted the dropmulalphabeta branch January 14, 2020 21:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants