Repository navigation
Conversation
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
A critical ratio-contrast naming issue remains unresolved in R/format.R.
1 open finding
What changed in this PR
Fixes marginaleffects output-column name clashes and enables p_adjust methods in estimate_slopes().
Changes:
- Preserves focal columns during formatting.
- Adjusts unformatted marginaleffects p-values.
- Adds regression tests, NEWS updates, and a version bump.
| File | Summary |
|---|---|
tests/testthat/test-focal-name-clashes.R |
Tests focal-name clashes and contrast formatting. |
tests/testthat/test-estimate_slopes.R |
Tests slope p-adjustment methods and edge cases. |
R/p_adjust.R |
Adjusts unformatted marginaleffects p-values. |
R/format.R |
Handles focal/output naming conflicts and contrast formatting. |
NEWS.md |
Documents the fixes. |
DESCRIPTION |
Bumps the development version. |
🧠 Review effort: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This branch has not been deployed
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.

This follows #679. With the marginaleffects backend, a variable named like an output column still gave wrong output. This happened in contrasts, custom hypotheses and contrasts of slopes. Also, most
p_adjustoptions ofestimate_slopes()did not work.Name clashes (all with
lm()and factors):contrastvariable namedDifference,torpchanged the output column with that name. The column became a factor of NA values, moved to the front, or both.comparison(for example"b2=b1"), abyvariable namedDifference,torpdid the same.estimate_slopes(comparison = "b2=b1"). There, the names wereSlope,SE,CI_low,CI_high,tandp.byvariable namedSlopegave twoDifferencecolumns. Withcomparison = "ratio", abyvariable namedRatioorSlopegave twoRatiocolumns.The cause was in
.standardize_marginaleffects_columns()(R/format.R).data_restoretype()converted each output column to the type of the variable with the same name. The reorder moved such columns to the front. Both now apply only to focal terms that are columns of the marginaleffects output. The renameSlopetoDifferenceinformat.marginaleffects_contrasts()now runs only for contrasts of slopes, whereSlopeis the estimate. For ratio contrasts, the suffix check now treatsRatioas an output name. Abyvariable namedRatiobecomesRatio.1.p_adjustinestimate_slopes(): eight methods failed with "replacement has 0 rows". These are "holm", "hochberg", "hommel", "bonferroni", "BH", "BY", "fdr" and "sidak". "tukey" returned unadjusted p-values ("Tukey adjustment requires at least 2 groups").get_marginaltrends()calls.p_adjust()on the unformatted marginaleffects output. That output hasp.valueandstatistic, but nopcolumn..p_adjust()now readsp.valueandstatisticfor that output, and counts one test per row. For formatted output (contrasts), it reads the columns as before. Abyvariable namedpnow keeps its values (asp.1), and the p-values are adjusted.Tests:
test-focal-name-clashes.R: a grid over 17 names and the 6 roles above. The 26 cells that stop with the rename error from Informative errors for variable names that marginaleffects overwrites (marginaleffects backend) #679 still stop with it. Each of the other 76 cells is compared with the same call for a variable namedsafe_var. The test compares the column names in order and every column. There are also tests forratio ~ pairwiseand forp_adjustin contrasts ofestimate_relation()output.test-estimate_slopes.R: eachp_adjustmethod againststats::p.adjust(),1 - (1 - p)^kandptukey(), for a numeric and a factorby. Also abyvariable namedp, and one test per row for a single custom hypothesis and for an ordinal model.Not fixed here: Bayesian models and inequality comparisons. Models other than
lm()were not checked.byfilters in contrasts of slopes fail for any variable name.devtools::test()gives 0 failures and 0 errors locally.devtools::check()gives 0 errors and 0 warnings. The version is bumped to 0.17.0.16.