Skip to content

Forward type, detrend, and size_title in check_model() methods for converted and wrapped models - #989

Open
jmgirard wants to merge 16 commits into
easystats:mainfrom
jmgirard:check-model-forwarding
Open

jmgirard wants to merge 16 commits into
easystats:mainfrom
jmgirard:check-model-forwarding

Conversation

@jmgirard

@jmgirard jmgirard commented Oct 18, 2026 •

Copy link
Copy Markdown
Contributor

Three methods call check_model() again on the inner model: check_model.stanreg() (also used for brms), check_model.model_fit() (parsnip), and check_model.performance_simres() (also used for DHARMa). They passed type = type and detrend = detrend with their own defaults. So missing(type) and missing(detrend) were always FALSE in check_model.default(), and its defaults never applied. They also did not pass size_title, so a user value was dropped.

For example, check_model(glm(am ~ wt, data = mtcars, family = binomial)) uses type = "discrete_interval" and detrend = FALSE. But check_model(simulate_residuals(<same glm>)) used "density" and TRUE. The detrend help already says simulated residuals are not detrended by default.

What changes

  • If the user gives no type or detrend, the three methods pass NULL, and check_model.default() treats NULL like a missing argument. They now pass size_title.
  • When the brms conversion fails, the fallback path also gives a NULL type its default. It turns a NULL detrend into TRUE, the value a direct call gives, so the result keeps a logical detrend.
  • NEWS entry, parsnip added to inst/WORDLIST, and the dev version is bumped.

Tests

  • simulate_residuals() of a binomial and a poisson glm and DHARMa::simulateResiduals() get "discrete_interval" and the default detrend. So do a model_fit object and the stanreg_glm_1 fixture, also inside a model_fit. The user's type, detrend, and size_title values are kept.
  • A wrapped lm keeps "density" and title_size 12. A wrapped brms model whose conversion fails keeps a logical detrend.

On main, the new type, size_title, and detrend expectations fail. Locally the full suite fails only test_performance - lavaan in test-test_performance.R, which also fails on main. R CMD check gives no warning.

@jmgirard
jmgirard requested a lite review from Copilot October 18, 2026 16:42

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

3 open issues
What changed in this PR

This PR fixes argument forwarding in check_model() methods that wrap/convert models so that check_model.default() can correctly apply its own defaults for type and detrend, and so user-specified size_title is preserved.

Changes:

  • Pass NULL for type / detrend from wrapper methods when the user didn’t supply them, and interpret NULL like “missing” in check_model.default().
  • Ensure size_title is forwarded through wrappers (stanreg, model_fit, and simulated residual objects).
  • Add regression tests + NEWS entry; bump development version and update WORDLIST.
File Description
R/​check_model.R Treat NULL like missing for type/detrend, forward size_title, adjust Stan fallback handling.
tests/​testthat/​test-check_model.R Add regression tests for forwarding/default behavior across wrapped/converted model types.
NEWS.md Document behavior change for defaults and size_title forwarding.
inst/​WORDLIST Add parsnip to spell-check allowlist.
DESCRIPTION Bump package version.

Review effort: Lite

Comment thread tests/testthat/test-check_model.R Outdated
Comment thread R/check_model.R
Comment thread R/check_model.R
@jmgirard

Copy link
Copy Markdown
Contributor Author

Related, but not changed here: with bayestestR 0.19.0, which the current minimum allows, check_model() on a multivariate brms model fails to convert it ("unused argument (resp = resp)") and falls back to the Bayesian checks. bayestestR 0.20.0 on CRAN has resp, so a minimum of 0.20.0 would avoid this, if you want that.

@strengejacke

Copy link
Copy Markdown
Member

Yes, we can do that. I just didn't want to increase version requirement for the recent performance submission because bayestestR was just released on CRAN, and sometimes, it is good to allow users some time to manage their software and package updates :-)

@strengejacke

Copy link
Copy Markdown
Member

Is there anything we need to change now that we set the minimum requirement to 0.20.0 for bayestestR?, Else, you can merge this PR

This branch has not been deployed

No deployments
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.

3 participants