Simplify contrasts of avg slopes#611
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the modelbased package to version 0.14.0.9, introducing a more intuitive syntax for calculating "context effects" (contrasts of average slopes) in estimate_contrast(). Instead of using specific string options for the comparison argument, users can now specify multiple numeric predictors directly in the contrast argument. The feedback highlights critical bugs in R/get_marginalcontrasts.R where the new second_focal variable is not properly initialized for single-predictor cases, which would cause the function to crash. There is also a request to clarify the documentation regarding the formula syntax for comparisons to ensure consistency between the description and the examples provided.
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request refactors the handling of context effects (contrasting average slopes) in estimate_contrast(). The previous requirement to explicitly set comparison = "slope" has been replaced by an automatic detection mechanism that triggers when two numeric predictors are passed to the contrast argument. Documentation, examples, and internal logic across several files have been updated to support this new syntax. Review feedback highlighted a bug involving an invalid regex backreference in variable name cleaning and a logic error where existing grouping variables in the by argument could be unintentionally overwritten.
There was a problem hiding this comment.
Pull request overview
This PR updates how estimate_contrasts() handles “context effects” / contrasts of average slopes, shifting from special comparison= modes to a simpler interface driven by the contrast argument (two numeric predictors, optionally plus a third grouping variable).
Changes:
- Simplifies context-effects usage so contrasting two average slopes is triggered by providing two numeric predictors in
contrast(and optionally a third variable for pairwise comparisons across levels). - Refactors internal slope/context detection (
contrast_slopes) and removes now-unneeded special-casing/mappings for prior slope/context comparison modes. - Updates documentation + tests to reflect the new interface and bumps the dev version.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/testthat/test-estimate_contrasts_context.R | Updates tests to use the new “two numeric predictors in contrast” API and checks the informational message when comparison is overridden. |
| tests/testthat/test-estimate_contrasts.R | Cleans up snapshot calls (removes # nolint markers around closing parentheses). |
| man/get_emmeans.Rd | Documents contrasting average slopes via contrast (but currently contains a malformed inline example). |
| man/estimate_contrasts.Rd | Same documentation update as above (also currently contains a malformed inline example). |
| R/get_marginalmeans.R | Removes a now-unnecessary early-return special case for context comparisons. |
| R/get_marginalcontrasts.R | Refactors slope/context detection into .get_marginaleffects_hypothesis_argument() and introduces contrast_slopes plumbing. |
| R/get_inequalitycontrasts.R | Switches from compute_slopes to my_args$contrast_slopes and removes mapping of "slope"/"slope_pairwise" to context modes. |
| R/format.R | Renames attribute usage from compute_slopes to contrast_slopes for inequality formatting logic. |
| R/estimate_contrasts.R | Expands docs: new note + new section on context effects via contrasting average slopes; updates examples accordingly. |
| NEWS.md | Updates release notes for the new API, but currently references a non-existent estimate_contrast() function name. |
| DESCRIPTION | Bumps package version to 0.14.0.9. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| levels at which contrasts are evaluated (e.g., \code{contrast="x=c('a','b')"}). | ||
| \strong{Note:} It is also possible to contrast average slopes, i.e. \code{contrast} can | ||
| be the name of two numeric predictors. However, while it is possible to filter | ||
| data for one numeric contrast (e.g., \code{contrast = c("num_pred=c(0, 1, 3)")}, |
There was a problem hiding this comment.
The contrast argument note contains a malformed example: \code{contrast = c("num_pred=c(0, 1, 3)")}, includes a literal } and trailing comma inside the \code{} block, which will display incorrectly in the rendered documentation. Please fix the example text so the code is valid and does not include stray punctuation (this likely should be regenerated from corrected roxygen comments).
| data for one numeric contrast (e.g., \code{contrast = c("num_pred=c(0, 1, 3)")}, | |
| data for one numeric contrast (e.g., \code{contrast = c("num_pred=c(0, 1, 3)")}), |
| levels at which contrasts are evaluated (e.g., \code{contrast="x=c('a','b')"}). | ||
| \strong{Note:} It is also possible to contrast average slopes, i.e. \code{contrast} can | ||
| be the name of two numeric predictors. However, while it is possible to filter | ||
| data for one numeric contrast (e.g., \code{contrast = c("num_pred=c(0, 1, 3)")}, |
There was a problem hiding this comment.
The contrast argument note contains a malformed example: \code{contrast = c("num_pred=c(0, 1, 3)")}, includes a literal } and trailing comma inside the \code{} block, so it will render incorrectly. Please correct the example (ideally by fixing the roxygen source and re-running roxygen2).
| data for one numeric contrast (e.g., \code{contrast = c("num_pred=c(0, 1, 3)")}, | |
| data for one numeric contrast (e.g., \code{contrast = c("num_pred=c(0, 1, 3)")}), |
| * Added option `comparison = "slope"` to `estimate_contrast()`, to allow | ||
| calculating contrasts of average slopes. This can also be useful when | ||
| estimating the context effects of within- and between-effects in a model. | ||
| * `estimate_contrast()` can now also calculate contrasts of average slopes, by |
There was a problem hiding this comment.
NEWS.md refers to estimate_contrast(), but the exported API appears to be estimate_contrasts() (plural). Please update this entry to the correct function name to avoid confusing users.
| * `estimate_contrast()` can now also calculate contrasts of average slopes, by | |
| * `estimate_contrasts()` can now also calculate contrasts of average slopes, by |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #611 +/- ##
==========================================
+ Coverage 89.75% 90.10% +0.35%
==========================================
Files 41 42 +1
Lines 3669 3842 +173
==========================================
+ Hits 3293 3462 +169
- Misses 376 380 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
No description provided.