-
Notifications
You must be signed in to change notification settings - Fork 48
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
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Codecov ReportPatch coverage:
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
☔ View full report in Codecov by Sentry. |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
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 That is, we can probably live with a compat entry in MixedModels.jl allowing 0.11,0.12 for BSplineKit. |
There was a problem hiding this 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
orfixefnames
, but that is the type of polish that we can make in a smaller, more focused PR.
- The exact internal structure of
- 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.
Co-authored-by: Phillip Alday <[email protected]>
Co-authored-by: Phillip Alday <[email protected]>
Co-authored-by: Phillip Alday <[email protected]>
Co-authored-by: Phillip Alday <[email protected]>
Co-authored-by: Phillip Alday <[email protected]>
Co-authored-by: Phillip Alday <[email protected]>
Co-authored-by: Phillip Alday <[email protected]>
Co-authored-by: Phillip Alday <[email protected]>
Co-authored-by: Phillip Alday <[email protected]>
Co-authored-by: Phillip Alday <[email protected]>
Co-authored-by: Phillip Alday <[email protected]>
Co-authored-by: Phillip Alday <[email protected]>
Co-authored-by: Phillip Alday <[email protected]>
Co-authored-by: Phillip Alday <[email protected]>
Co-authored-by: Phillip Alday <[email protected]>
Co-authored-by: Phillip Alday <[email protected]>
@dmbates I think I've messed up the indents on some of the |
fixed |
@dmbates merge and release when ready 🚀 |
profile
method forLinearMixedModel
currently profiles w.r.t.σ
, the fixed-effects coefficients, theσ
parameters, and theθ
parameters (but not the correlation parameters).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 aVector{NamedTuple}
. TheTypedTables.Table
generic is re-exported from this package to allow for presenting this row-table in a table format.Did behavior change? Did you add need features? If so, please update NEWS.md
docs/NEWS-update.jl
to update the cross-references.Should we release your changes right away? If so, bump the version: