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

Profile the objective #639

Merged
merged 103 commits into from May 4, 2023
Merged

Profile the objective #639

merged 103 commits into from May 4, 2023

Conversation

dmbates
Copy link
Collaborator

@dmbates dmbates commented Sep 21, 2022

  • The profile method for LinearMixedModel currently profiles w.r.t. σ, the fixed-effects coefficients, the σ parameters, and the θ parameters (but not the correlation parameters).
  • The MixedModelProfile struct returned contains a table, tbl, of values of ζ (the signed square root of the change in objective from the optimum) and β and θ as a Vector{NamedTuple}. The TypedTables.Table generic is re-exported from this package to allow for presenting this row-table in a table format.
  • Forward and reverse interpolation splines are added for parameters to and from ζ

Did behavior change? Did you add need features? If so, please update NEWS.md

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

Should we release your changes right away? If so, bump the version:

  • I've bumped the version appropriately

@dmbates dmbates marked this pull request as draft September 21, 2022 21:35
@dmbates

This comment was marked as outdated.

@codecov
Copy link

codecov bot commented Sep 21, 2022

Codecov Report

Patch coverage: 94.04% and project coverage change: -0.26 ⚠️

Comparison is base (d946f6f) 95.90% compared to head (8977dc1) 95.64%.

❗ Current head 8977dc1 differs from pull request most recent head f8f4559. Consider uploading reports for the commit f8f4559 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #639      +/-   ##
==========================================
- Coverage   95.90%   95.64%   -0.26%     
==========================================
  Files          29       35       +6     
  Lines        2732     3189     +457     
==========================================
+ Hits         2620     3050     +430     
- Misses        112      139      +27     
Impacted Files Coverage Δ
src/MixedModels.jl 100.00% <ø> (ø)
src/utilities.jl 96.92% <ø> (ø)
src/linearmixedmodel.jl 94.88% <28.57%> (-3.23%) ⬇️
src/profile/utilities.jl 94.31% <94.31%> (ø)
src/profile/thetapr.jl 97.14% <97.14%> (ø)
src/remat.jl 96.69% <97.22%> (-0.04%) ⬇️
src/bootstrap.jl 93.93% <100.00%> (+2.15%) ⬆️
src/profile/fixefpr.jl 100.00% <100.00%> (ø)
src/profile/profile.jl 100.00% <100.00%> (ø)
src/profile/sigmapr.jl 100.00% <100.00%> (ø)
... and 1 more

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@palday

This comment was marked as outdated.

Project.toml Show resolved Hide resolved
@dmbates

This comment was marked as outdated.

@palday

This comment was marked as outdated.

@dmbates
Copy link
Collaborator Author

dmbates commented Oct 18, 2022

If you have time to look at that, sure. I expect that the periodic splines, which were introduced in BSplineKit v0.12.0, will be needed in MixedModelsMakie but probably not in MixedModels.

The way I imagine the division of responsibility is that MixedModels will add the forward and backward splines to the MixedModelProfile, because they are used to obtain profile-based confidence intervals. These splines are natural cubic splines. The reverse splines, mapping the zeta values to the fixed-effects coefficients, are used to create these profile-based confidence intervals. The periodic splines are used for contour interpolation, which should be in MixedModelsMakie, I think.

That is, we can probably live with a compat entry in MixedModels.jl allowing 0.11,0.12 for BSplineKit.

test/pls.jl Outdated Show resolved Hide resolved
test/pls.jl Outdated Show resolved Hide resolved
test/pls.jl Outdated Show resolved Hide resolved
test/pls.jl Outdated Show resolved Hide resolved
Copy link
Member

@palday palday left a comment

Choose a reason for hiding this comment

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

I think this is mostly good to go for an initial merge and release.

  • I've marked all comment-conversations as resolved that I think are nonblocking.
  • I've added a number of docstring suggestions that also highlight where the API guarantees are and which things might still change in the 4.x series. There are three broad things that I think we might revisit:
    • The exact internal structure of MixedModelProfile, but I think that is not really interesting for most users, who just want the profile to compute the CI.
    • The concrete Tables.jl-compatible return types. I don't really have any objection to the current DictTable, but I suspect that we'll only really know what works as we get more experience with using this feature in practice.
    • The parameter names in the various tables. For the fixed effects, I would really like to see these match coefnames or fixefnames, but that is the type of polish that we can make in a smaller, more focused PR.
  • Related to nonblocking polish, I've opened up a discussion for that: Polish for profiling #688.
  • I've added a NEWS entry that tries to highlight the "awesome new feature but some details might change!" nature of the release. Feel free to modify.
  • I've bumped the version appropriately.

Feel free to keep or reject all my suggestions.

dmbates and others added 17 commits May 4, 2023 09:50
Co-authored-by: Phillip Alday <palday@users.noreply.github.com>
Co-authored-by: Phillip Alday <palday@users.noreply.github.com>
Co-authored-by: Phillip Alday <palday@users.noreply.github.com>
Co-authored-by: Phillip Alday <palday@users.noreply.github.com>
Co-authored-by: Phillip Alday <palday@users.noreply.github.com>
Co-authored-by: Phillip Alday <palday@users.noreply.github.com>
Co-authored-by: Phillip Alday <palday@users.noreply.github.com>
Co-authored-by: Phillip Alday <palday@users.noreply.github.com>
Co-authored-by: Phillip Alday <palday@users.noreply.github.com>
Co-authored-by: Phillip Alday <palday@users.noreply.github.com>
Co-authored-by: Phillip Alday <palday@users.noreply.github.com>
Co-authored-by: Phillip Alday <palday@users.noreply.github.com>
Co-authored-by: Phillip Alday <palday@users.noreply.github.com>
Co-authored-by: Phillip Alday <palday@users.noreply.github.com>
Co-authored-by: Phillip Alday <palday@users.noreply.github.com>
Co-authored-by: Phillip Alday <palday@users.noreply.github.com>
@palday
Copy link
Member

palday commented May 4, 2023

@dmbates I think I've messed up the indents on some of the !!! note blocks. When you're all done, I make those changes locally and push before you merge.

@palday
Copy link
Member

palday commented May 4, 2023

@dmbates I think I've messed up the indents on some of the !!! note blocks. When you're all done, I make those changes locally and push before you merge.

fixed

@palday
Copy link
Member

palday commented May 4, 2023

@dmbates merge and release when ready 🚀

@dmbates dmbates merged commit 25ed4e8 into main May 4, 2023
8 checks passed
@dmbates dmbates deleted the profile branch May 4, 2023 18:40
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.

None yet

3 participants