Don't match auxiliary parameter names fuzzily - #1225
Open
DominiqueMakowski wants to merge 1 commit into
Open
Conversation
`find_auxiliary()` detected the residual SD by looking for the string
"sigma" anywhere in the names of the stan-model's parameters. Custom
brms-families whose auxiliary parameters merely *contain* that string
(e.g. the "sigmadrift", "sigmabias" and "sigmandt" parameters of the
`ddm()` family from *cogmod*) hence produced a "sigma" component that
does not exist in the model. Sigma is now matched exactly, and only
when it isn't already covered by the model's "pforms": as "sigma" for
univariate models, "sigma_<response>" for multivariate models, and
"sigma1", "sigma2" etc. for mixture models.
`clean_parameters()` had the same problem when mapping the elements of
`find_parameters()` to components: the element named "sigmabias" matched
`grepl("sigma", ...)`, so its parameters were lumped into the "sigma"
component, although `find_parameters()` correctly returned the two as
separate elements. Component names are now compared for exact matches,
after stripping the "_random" suffix that marks the group-level part of
a component. This also fixes the related grouping in
easystats/parameters#1247.
Fixes #1224
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The changes are narrowly scoped, improve correctness without altering intended outputs for valid sigma parameters, and are backed by targeted regression tests.
Pull request overview
This PR fixes incorrect fuzzy matching of auxiliary/distributional parameter names in brms models, which previously caused "sigma" to be detected/assigned when parameter names merely contained the substring "sigma" (e.g. "sigmabias"), addressing insight#1224 and the downstream grouping issue seen in easystats/parameters#1247.
Changes:
- Tighten
find_auxiliary.brmsfit()to add"sigma"only when an actual sigma parameter is present (univariate, multivariatesigma_<response>, or mixturesigma1,sigma2, …) and not already covered bypforms. - Make
.get_stan_params()(used byclean_parameters()) map components via exact name matching after stripping the"_random"suffix, eliminating substring-based component lumping. - Add focused unit tests with lightweight
brmsfitmock objects; document the fixes inNEWS.mdand bump the dev version.
File summaries
| File | Description |
|---|---|
| tests/testthat/test-brms_dpars.R | Adds unit tests that reproduce and prevent the sigma substring mis-detection/regression without requiring fitted/downloaded models. |
| R/find_auxiliary.R | Replaces fuzzy "sigma" detection with exact matching via a dedicated .brms_has_sigma() helper. |
| R/clean_parameters.R | Switches component mapping to exact matches after removing "_random" suffix, preventing mis-grouping of custom auxiliary parameters. |
| NEWS.md | Notes both bug fixes in the current (devel) section. |
| DESCRIPTION | Bumps package dev version to 1.5.4.1 for the user-visible bug fix. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1224, and the related component grouping in easystats/parameters#1247.
find_auxiliary()hallucinatedsigmaThe residual SD was detected by looking for the string
"sigma"anywhere in the stan-model's parameter names:Custom brms-families whose auxiliary parameters merely contain that string therefore faked a
sigmacomponent. Theddm()family from cogmod estimatessigmadrift,sigmabiasandsigmandtas constants, so:sigmais now matched exactly, and only when it isn't already covered by the model'spforms:sigma— univariate models with a constant residual SDsigma_<response>— multivariate models, validated against the actual response names (both the raw and brms' "cleaned" ones, e.g.sigma_SepalLengthforSepal.Length)sigma1,sigma2, … — mixture modelsclean_parameters()lumpedsigmabiasintosigmaSame root cause, different code path.
.get_stan_params()mappedfind_parameters()element names to components with a substring test, so the element namedsigmabiasmatchedgrepl("sigma", ...)and its parameters were labelledComponent = "sigma"— even thoughfind_parameters()had correctly returnedsigmaandsigmabiasas separate elements. That is what surfaces as easystats/parameters#1247;parametersitself needed no change, its output is downstream ofclean_parameters().Component names are now compared for exact matches, after stripping the
_randomsuffix that marks the group-level part of a component. Thegreplbranches forconditional,priors,smooth_termsanddispersionbecome redundant once that suffix is stripped, since they fall through to the identity default — so they are gone too, which removes the same fuzziness for those.Note this model genuinely has an exact
sigmaparameter (fixed to 1), so thefind_auxiliary()fix alone did not address it; it still shows up as its ownsigmacomponent, correctly.Verification
find_auxiliary()output compared across 19 downloadable brms models (univariate, mv with dotted response names, mixture, zi, zoib, ordinal, distributionalsigma ~ x,sigmawith random effects, customchocominifamily) — identical in every case.tests/testthat/test-brms_dpars.Rcovers both bugs with lightweightbrmsfitmockups (formula + stan parameter names), so no fitting or downloading is needed; the mockups reproduce the real models' output exactly. It also unit-tests the.get_stan_params()component mapping over the element names produced by all five classes that share the helper —brmsfit,stanreg,stanfit,stanmvreg,bamlss— since rstanarm isn't available locally to exercise those paths end-to-end.GFIvalues intest-export_table.R, andrt/RTintest-get_predicted.R:904) reproduce unchanged onmain.parameters' ownbrms|stan|bayestests pass against this branch.🤖 Generated with Claude Code