Add a progress meter for LMM fits #539

dmbates · 2021-07-07T23:15:22Z

This is just a proof of concept but it seems to work well (once I read the documentation for ProgressMeter and read our code a little more carefully).
Try, for example

julia> using MixedModels

julia> using MKL

julia> mrk17 = MixedModels.dataset(:mrk17_exp1);

julia> form = @formula 1000 / rt ~ 1 + F*P*Q*lQ*lT + (1+F+P+Q+lQ+lT|subj) + (1+P+Q+lQ+lT|item);

julia> contr = merge(Dict(nm => EffectsCoding() for nm in (:F, :P, :Q, :lQ, :lT)), Dict(nm => Grouping() for nm in (:subj, :item)));

julia> m = fit(MixedModel, form, mrk17; contrasts=contr);
Progress:  1316  Time: 0 Time: 0:00:07 ( 5.53 ms/it)

During the iterations the current objective is displayed below the Progress line,

Not entirely sure how the Time: 0 ends up in that line.

Project.toml

src/linearmixedmodel.jl

palday · 2021-07-08T00:47:37Z

The CI failures seem to be a problem with the Windows VMs.

palday · 2021-07-08T00:48:53Z

I think I had looked at making the iterations clear and print in the same spot, but then got sidetracked: timholy/ProgressMeter.jl#204

dmbates · 2021-07-08T14:26:01Z

CI is failing for Julia 1.4 on Windows because it can't download packages correctly. Should we switch the CI to julia 1.6 only? (Until we are able to run tests on 1.7 and later.)

palday · 2021-07-08T14:32:26Z

Since this is for the breaking 4.0 release, let's just go ahead and make support start at 1.6. Can you also update Project.toml? (And even though it's not a feature, a note to NEWS?)

dmbates · 2021-07-08T14:51:11Z

Will the changes re CI on version 1.6 (and, sigh!, add a news item - I feel like a surly teenager rolling his eyes about the stupid rules his parents have) when I get back from a walk.

palday · 2021-07-08T14:53:55Z

@dmbates I'll take care of it while you're out -- I have a minute while a computation runs. 😄

codecov · 2021-07-08T15:09:09Z

Codecov Report

Merging #539 (491a4d1) into main (d6e0669) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #539      +/-   ##
==========================================
+ Coverage   94.28%   94.29%   +0.01%     
==========================================
  Files          26       26              
  Lines        2308     2313       +5     
==========================================
+ Hits         2176     2181       +5     
  Misses        132      132

Impacted Files	Coverage Δ
src/bootstrap.jl	`97.84% <100.00%> (ø)`
src/generalizedlinearmixedmodel.jl	`90.20% <100.00%> (+0.10%)`	⬆️
src/linearmixedmodel.jl	`95.28% <100.00%> (+0.01%)`	⬆️

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 d6e0669...491a4d1. Read the comment docs.

dmbates · 2021-07-08T16:44:34Z

Should the default value of verbose be changed to true? I think this display is informative and may help educate users as to what is making their model fits slow. On the other hand, I don't want to clutter up users' sessions with unwanted displays.

dmbates · 2021-07-08T17:16:59Z

There is a problem with using the optional argument verbose for generalized linear mixed models in that it also causes the deviance evaluation to become verbose. As a temporary fix I will add an optional progress argument separate from verbose but that is just to get things working. We can decide on a more general approach later.

palday · 2021-07-08T17:24:46Z

What does verbose output look like now for a quick fit like sleepstudy?

dmbates · 2021-07-08T17:38:35Z

What does verbose output look like now for a quick fit like sleepstudy?

For different machines it could be that nothing shows up or just the final summary. Perhaps we should change the optional argument name to progress throughout and explain in the documentation that this uses the facilities of the ProgressMeter package which doesn't do anything if the minimization time is much less than 1 second.

By the way, I looked in the ProgressMeter package for a NEWS.md file to check on what version is needed for which capabilities and there isn't a NEWS.md file. So there!

palday · 2021-07-08T18:57:39Z

I like the idea of changing the argument name to progress.

By the way, I looked in the ProgressMeter package for a NEWS.md file to check on what version is needed for which capabilities and there isn't a NEWS.md file. So there!

😁

dmbates · 2021-07-10T17:06:44Z

@palday Can you take another look, please?

src/generalizedlinearmixedmodel.jl

palday · 2021-07-11T23:05:59Z

I think it's nearly there -- what do you think of my comments for verbose vs. progress in GLMM?

Co-authored-by: Phillip Alday <[email protected]>

dmbates · 2021-07-12T14:04:18Z

I think it's nearly there -- what do you think of my comments for verbose vs. progress in GLMM?

Those look good, thanks. Both committed now.

src/linearmixedmodel.jl

dmbates · 2021-07-12T17:22:50Z

@palday Took a bit longer than I imagined as I needed to add a default to refit! methods as well. I chose progress::Bool=false for refit!. I also replaced a couple of occurrences of the now deprecated model_response with response in the tests.

src/linearmixedmodel.jl

palday · 2021-07-12T17:24:20Z

@dmbates while you were writing that comment, I was already commenting on the choice of default for refit!. I'm fine with false as the default, but true would be consistent with fit!

dmbates · 2021-07-12T17:38:10Z

@dmbates while you were writing that comment, I was already commenting on the choice of default for refit!. I'm fine with false as the default, but true would be consistent with fit!

My reasoning was that for a refit! the user has already had the chance to see the progress meter for the optimization so the novelty may have worn off. I would be okay with changing the default to true for consistency with fit! but then we would need to go through all the calls to refit! in the tests to add progress=false to them. Let me know if you want me to do that.

palday · 2021-07-12T17:41:13Z

I had a similar thought -- typically the user knows better what to expect with refit and model is already constructed and everything is precompiled, so that bit of overhead is gone.

We could take a middle ground that will make things easier in the future if we expose more options to the user: have a kwargs... to refit! that is splatted into the call to fit! and then the defaults are just carried over.

dmbates · 2021-07-12T17:44:42Z

@palday the kwargs... approach sounds good. Would you be willing to make that change?

palday · 2021-07-12T17:44:51Z

yep, will do

palday · 2021-07-12T18:48:44Z

test/pirls.jl

-        refit!(gm2r, 1 .- gm2.y; fast=true)
-        @test gm2r.β ≈ -gm2.β atol=1e-3
+
+        refit!(gm2r; fast=true, progress=false)
+        @test length(gm2r.optsum.final) == 1
        @test gm2r.θ ≈ gm2.θ atol=1e-3

-        refit!(gm2r, 1 .- gm2.y; fast=false)
+        # swapping successes and failures to give us the same model
+        # but with opposite signs. healthy ≈ 1 - response(gm2r)
+        # but defining it in terms of the original values avoids some
+        # nasty floating point issues
+        healthy = @. (cbpp.hsz - cbpp.incid) / cbpp.hsz
+        refit!(gm2r, healthy; fast=false, progress=false)
+        @test length(gm2r.optsum.final) == 5


@dmbates going the kwargs... route made me realize that we weren't passing all the arguments here ... which then lead to the discovery that the "flipped" response was numerically pathological for fast fits. Hence the changes here.

palday

As soon as the CIs pass, please merge. 😄

dmbates · 2021-07-13T00:59:57Z

@palday I seem to be playing "whack-a-mole" again trying to track down all the instances of fit( or refit!( in the tests and adding progress=false. The tests on Mac OSX indicate that there may be a problem with the bootstrap tests now. I feel that the parametricbootstrap function should turn off the progress meter unconditionally.

Does it seem that we are going about this the wrong way? I don't mind a bit of whack-a-mole but this seems to be going on longer than is wise.

By the way, the Mac tests seem to be slower and hence better at finding cases where the progress meter kicks in.

palday · 2021-07-13T01:23:43Z

Interesting on the Mac tests ....

I agree that parametricbootstrap should unconditionally set progress=false -- it has its own progress meter anyway.

I think there's not much to be done about the whack-a-mole -- that's just what happens when you change defaults. 🤷 I have a somewhat slower computer I can also run the tests on and see if that reveals any more cases.

Add a progress meter for LMM fits

6ff3027

dmbates requested a review from palday July 7, 2021 23:15

Remove unnecessary iteration counter

dbf1c09

palday reviewed Jul 8, 2021

View reviewed changes

Project.toml Outdated Show resolved Hide resolved

palday reviewed Jul 8, 2021

View reviewed changes

src/linearmixedmodel.jl Outdated Show resolved Hide resolved

require at least julia 1.6

8e38b17

Add progress meter for GLMMs. Change format.

e8d0030

dmbates added 3 commits July 9, 2021 11:42

Rename verbose to progress, change default to true

cbd6908

Expand on NEWS entry for #539

5696c91

Back-out changes to compat for ProgressMeter

8c3ddff

palday reviewed Jul 11, 2021

View reviewed changes

src/generalizedlinearmixedmodel.jl Show resolved Hide resolved

palday reviewed Jul 11, 2021

View reviewed changes

src/generalizedlinearmixedmodel.jl Outdated Show resolved Hide resolved

palday reviewed Jul 11, 2021

View reviewed changes

src/generalizedlinearmixedmodel.jl Show resolved Hide resolved

dmbates and others added 2 commits July 12, 2021 09:03

Update src/generalizedlinearmixedmodel.jl

b002ad2

Co-authored-by: Phillip Alday <[email protected]>

Update src/generalizedlinearmixedmodel.jl

4c1adcf

Co-authored-by: Phillip Alday <[email protected]>

dmbates added 2 commits July 12, 2021 12:12

Add progress arg to refit! with default of false.

20807e7

progress=false in fits; model_response -> response

5a31117

palday reviewed Jul 12, 2021

View reviewed changes

src/linearmixedmodel.jl Outdated Show resolved Hide resolved

palday reviewed Jul 12, 2021

View reviewed changes

src/linearmixedmodel.jl Outdated Show resolved Hide resolved

palday reviewed Jul 12, 2021

View reviewed changes

src/linearmixedmodel.jl Outdated Show resolved Hide resolved

Missed one progress=false change

c602e67

palday added 5 commits July 12, 2021 12:55

ProgressMeter compat bump (needed for showspeed)

e15dd1c

kwargs... for refit!

bf56ca2

simplify second method

7cabb81

fix tests for latent floating point issue

a9d984a

remove debug code

55a6d38

palday reviewed Jul 12, 2021

View reviewed changes

palday approved these changes Jul 12, 2021

View reviewed changes

dmbates added 4 commits July 12, 2021 18:48

pass progress=false in refit! in tests

39c3d0c

More cases of adding progress=false in refit! call

0ee090f

add progress=false in more fit and refit! calls

384af96

yet another progress=false

8902b55

palday added 2 commits July 12, 2021 22:15

disable single-model progress in bootstrap

1222a51

whack-a-mole + ; before kwargs

491a4d1

palday merged commit b96d6c7 into main Jul 13, 2021

palday deleted the db/progressmeter branch July 13, 2021 03:34

palday mentioned this pull request Jul 13, 2021

Update verbose output to use ProgressMeter #513

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a progress meter for LMM fits #539

Add a progress meter for LMM fits #539

dmbates commented Jul 7, 2021

palday commented Jul 8, 2021

palday commented Jul 8, 2021

dmbates commented Jul 8, 2021 •

edited

Loading

palday commented Jul 8, 2021

dmbates commented Jul 8, 2021

palday commented Jul 8, 2021

codecov bot commented Jul 8, 2021 •

edited

Loading

dmbates commented Jul 8, 2021

dmbates commented Jul 8, 2021

palday commented Jul 8, 2021

dmbates commented Jul 8, 2021

palday commented Jul 8, 2021

dmbates commented Jul 10, 2021

palday commented Jul 11, 2021

dmbates commented Jul 12, 2021

dmbates commented Jul 12, 2021

palday commented Jul 12, 2021

dmbates commented Jul 12, 2021

palday commented Jul 12, 2021

dmbates commented Jul 12, 2021

palday commented Jul 12, 2021

palday Jul 12, 2021

palday left a comment

dmbates commented Jul 13, 2021 •

edited

Loading

palday commented Jul 13, 2021

Add a progress meter for LMM fits #539

Add a progress meter for LMM fits #539

Conversation

dmbates commented Jul 7, 2021

palday commented Jul 8, 2021

palday commented Jul 8, 2021

dmbates commented Jul 8, 2021 • edited Loading

palday commented Jul 8, 2021

dmbates commented Jul 8, 2021

palday commented Jul 8, 2021

codecov bot commented Jul 8, 2021 • edited Loading

Codecov Report

dmbates commented Jul 8, 2021

dmbates commented Jul 8, 2021

palday commented Jul 8, 2021

dmbates commented Jul 8, 2021

palday commented Jul 8, 2021

dmbates commented Jul 10, 2021

palday commented Jul 11, 2021

dmbates commented Jul 12, 2021

dmbates commented Jul 12, 2021

palday commented Jul 12, 2021

dmbates commented Jul 12, 2021

palday commented Jul 12, 2021

dmbates commented Jul 12, 2021

palday commented Jul 12, 2021

palday Jul 12, 2021

Choose a reason for hiding this comment

palday left a comment

Choose a reason for hiding this comment

dmbates commented Jul 13, 2021 • edited Loading

palday commented Jul 13, 2021

dmbates commented Jul 8, 2021 •

edited

Loading

codecov bot commented Jul 8, 2021 •

edited

Loading

dmbates commented Jul 13, 2021 •

edited

Loading