Fix wrong re-ordering of b-row-indices - #659
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The updated placeholder approach still has a correctness edge case for comparisons involving more than 26 b indices and the new test should explicitly pin the backend to avoid reliance on global options.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes a bug in custom hypothesis re-indexing for estimate_contrasts() (marginaleffects backend) where coefficient references like b1 could be incorrectly substituted inside b10/b12, leading to wrong reordering/assignment of estimates. It also adds a regression test, and updates the changelog and package dev version accordingly.
Changes:
- Adjusted the re-indexing logic for custom comparisons to replace only whole
b#tokens (avoidingb1being substituted insideb10). - Added a test covering custom comparisons with
bindices above 9 to prevent regressions. - Added a NEWS entry and bumped the DESCRIPTION dev version.
File summaries
| File | Description |
|---|---|
| tests/testthat/test-estimate_contrasts.R | Adds a regression test validating custom comparison behavior for larger b indices against marginaleffects. |
| R/get_marginalcontrasts.R | Updates token replacement logic used to reorder custom b# hypotheses. |
| NEWS.md | Documents the bug fix under the (devel) section. |
| DESCRIPTION | Bumps package version to a development patch version. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
No description provided.