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 fitlog #552

Merged
merged 16 commits into from Aug 19, 2021
Merged

add fitlog #552

merged 16 commits into from Aug 19, 2021

Conversation

palday
Copy link
Member

@palday palday commented Aug 17, 2021

Addresses (but does not close) #549 (because we want to change OptSummary later to be smarter about this).

Closes #549. There is a minor inefficiency in that initial, final, finitial, fmin store information duplicated in the fitlog. Not duplicating this information (by cheating and using properties that redirect to the relevant field entries) turns out to make the bookkeeping a lot messier, so I've let this inefficiency remain (which also keeps better compatibility). I use Vector inside of the log instead of SVector because the changing size of the parameter vector in GLMM creates problems.

It also seems that I might have found another case where JuliaLang/julia#40048 isn't as fixed as we had hoped -- I get a big stackoverflow type inference dump as a Heisenbug when stepping through the contra testset, BUT it continues afterwards with the correct result.

@codecov
Copy link

codecov bot commented Aug 17, 2021

Codecov Report

Merging #552 (54c605d) into main (fd79f39) will decrease coverage by 0.20%.
The diff coverage is 80.64%.

❗ Current head 54c605d differs from pull request most recent head 3ca5bf5. Consider uploading reports for the commit 3ca5bf5 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main     #552      +/-   ##
==========================================
- Coverage   96.25%   96.05%   -0.21%     
==========================================
  Files          27       27              
  Lines        2431     2456      +25     
==========================================
+ Hits         2340     2359      +19     
- Misses         91       97       +6     
Impacted Files Coverage Δ
src/generalizedlinearmixedmodel.jl 89.83% <77.77%> (-0.37%) ⬇️
src/linearmixedmodel.jl 97.71% <80.95%> (-0.72%) ⬇️
src/optsummary.jl 97.72% <100.00%> (+0.05%) ⬆️

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 fd79f39...3ca5bf5. Read the comment docs.

@palday palday requested review from dmbates August 17, 2021 20:15
src/linearmixedmodel.jl Outdated Show resolved Hide resolved
@palday
Copy link
Member Author

palday commented Aug 18, 2021

@dmbates I took the opportunity to add a plot showing the different convergence pattern of Nelder-Mead and BOBYQA: https://juliastats.org/MixedModels.jl/previews/PR552/optimization/#Modifying-the-optimization-process

@@ -13,7 +13,11 @@ include("modelcache.jl")

@testset "contra" begin
contra = dataset(:contra)
gm0 = fit(MixedModel, only(gfms[:contra]), contra, Bernoulli(); fast=true, progress=false)
thin = 5
gm0 = fit(MixedModel, only(gfms[:contra]), contra, Bernoulli(); fast=true, progress=false, thin)
Copy link
Collaborator

Choose a reason for hiding this comment

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

That syntax leads me astray. I keep forgetting that it expands to , thin=thin)

Copy link
Collaborator

@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.

Thanks for making these changes. I think it is much improved.

@palday
Copy link
Member Author

palday commented Aug 19, 2021

@dmbates Are you happy with tagging a new release with this and constrained sigma? That locks us into this API until 5.0 lands.

@dmbates
Copy link
Collaborator

dmbates commented Aug 19, 2021 via email

@palday
Copy link
Member Author

palday commented Aug 19, 2021

Yep, easy.

@palday palday merged commit ace7f89 into main Aug 19, 2021
@palday palday deleted the pa/fitlog branch August 19, 2021 16:43
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.

Add fitlog kwarg for logging the parameter vector during optimization
2 participants