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

restore and save optsum for GLMM #791

Merged
merged 15 commits into from
Nov 8, 2024
Merged

restore and save optsum for GLMM #791

merged 15 commits into from
Nov 8, 2024

Conversation

palday
Copy link
Member

@palday palday commented Nov 5, 2024

closes #790

  • add entry in NEWS.md
  • after opening this PR, add a reference and run docs/NEWS-update.jl to update the cross-references.
  • I've bumped the version appropriately

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Copy link

codecov bot commented Nov 5, 2024

Codecov Report

Attention: Patch coverage is 97.43590% with 1 line in your changes missing coverage. Please review.

Project coverage is 96.93%. Comparing base (c0b9e2d) to head (61ed6f3).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/serialization.jl 97.05% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #791      +/-   ##
==========================================
- Coverage   96.94%   96.93%   -0.01%     
==========================================
  Files          33       33              
  Lines        3370     3397      +27     
==========================================
+ Hits         3267     3293      +26     
- Misses        103      104       +1     
Flag Coverage Δ
current 96.64% <97.43%> (+0.05%) ⬆️
minimum 96.87% <97.43%> (-0.01%) ⬇️
nightly 96.57% <97.43%> (+0.05%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@palday palday requested a review from ararslan November 5, 2024 16:51
Copy link
Member

@ararslan ararslan left a comment

Choose a reason for hiding this comment

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

I don't feel like I can give a super thoughtful review as I'm not familiar with this part of the codebase but here are some comments/suggestions in case they're useful, plus an approval since nothing seemed obviously wrong (as far as I could tell).

src/serialization.jl Outdated Show resolved Hide resolved
src/serialization.jl Outdated Show resolved Hide resolved
src/serialization.jl Outdated Show resolved Hide resolved
Comment on lines +53 to +54
if length(ops.lowerbd) != length(m.θ)
deleteat!(ops.lowerbd, 1:length(m.β))
Copy link
Member

Choose a reason for hiding this comment

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

I suppose it can never be the case that length(ops.lowerbd) < length(m.β) or that ops.lowerbd has indices that don't start at 1?

Copy link
Member Author

Choose a reason for hiding this comment

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

lowerbd is in a struct we control and it's always Vector, so the indices are fine.

There are two cases here:

  • the "slow" optimization, where the parameter vector contains both beta and theta, so length(lowerbd) needs to be length(beta) + length(theta).
  • the "fast" optimization, where the parameter vector contains only theta, so length(lowerbd) needs to be length(theta). (In this case, there is an inner optimization step that finds the best beta for a given theta using PIRLS -- this will generally give the same answer or a very close one to doing free optimization over both, but there are a few pathological cases where the two methods can diverge, in which case the extra freedom of the slow optimization will yield more accurate results.)

The initial model creation initializes the lower bound to the length of theta because that is shared with the initialization for the linear mixed model (where only theta is included in optimization and beta is solved for directly).

src/serialization.jl Outdated Show resolved Hide resolved
src/serialization.jl Outdated Show resolved Hide resolved
palday and others added 7 commits November 8, 2024 03:41
Co-authored-by: Alex Arslan <[email protected]>
Co-authored-by: Alex Arslan <[email protected]>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@palday palday merged commit f03c65b into main Nov 8, 2024
12 checks passed
@palday palday deleted the pa/save_glmm branch November 8, 2024 04:29
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.

What is current best practice for saving a GLMM?
2 participants