-
-
Notifications
You must be signed in to change notification settings - Fork 23
Add validation to ensure es_type parameter is only used with effectsize = "boot"
#577
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add validation to ensure es_type parameter is only used with effectsize = "boot"
#577
Conversation
Co-authored-by: rempsyc <13123390+rempsyc@users.noreply.github.com>
Co-authored-by: rempsyc <13123390+rempsyc@users.noreply.github.com>
es_type parameter when effectsize == "boot"es_type parameter is only used with effectsize = "boot"
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds validation to ensure that the es_type parameter can only be used when effectsize = "boot" in estimate_contrasts(), addressing issue #575. The change prevents silent parameter ignoring and provides clear error messages when the parameter is misused.
Key changes:
- Added validation logic using
match.call()to detect explicites_typeusage - Added comprehensive test coverage for both error and success cases
- Fixed a spelling error in documentation from "representiv" to "representative"
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| R/estimate_contrasts.R | Added validation logic to check es_type usage with appropriate effectsize values |
| tests/testthat/test-estimate_contrasts_effectsize.R | Added comprehensive test suite and reformatted existing tests for better readability |
| man/estimate_means.Rd | Fixed spelling error in documentation |
| inst/WORDLIST | Updated word list for spell checking |
Comments suppressed due to low confidence (1)
man/estimate_means.Rd:1
- Corrected spelling of 'representiv' to 'representative'.
% Generated by roxygen2: do not edit by hand
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| } else if (effectsize != "boot") { | ||
| insight::format_error("`es_type` can only be used when `effectsize = \"boot\"`.") | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should move these checks into the function that computes the effect sizes, .estimate_contrasts_effectsize().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But then we have a minor problem with the default effectsize = NULL (with es_type still specified) because then the checks get skipped and people will not see the error and therefore not understand why es_type is being ignored (or perceived as incorrect values):
modelbased/R/estimate_contrasts.R
Lines 366 to 376 in f2ba8b3
| if (!is.null(effectsize)) { | |
| out <- .estimate_contrasts_effectsize( | |
| model = model, | |
| estimated = estimated, | |
| contrasts_results = out, | |
| effectsize = effectsize, | |
| bootstraps = iterations, | |
| bootES_type = es_type, | |
| backend = backend | |
| ) | |
| } |
There's another area of confusion. If people are not allowed to manually specify es_type = "cohens.d" and be faced with the error, they might be confused as to why the defaultis es_type = "cohens.d" nonetheless? So if we change the default to es_type = NULL, then we possibly resolve this ambiguity and also the above problem (so checks can stay within .estimate_contrasts_effectsize()).
Summary
This PR adds validation to ensure that the
es_typeparameter can only be used wheneffectsize = "boot"inestimate_contrasts(), addressing issue #575.Problem
Previously, users could specify
es_typewith any value ofeffectsize, even though this parameter only applies wheneffectsize = "boot". This could lead to confusion as the parameter would be silently ignored for other effect size methods ("emmeans","marginal", orNULL).As noted in the issue:
Solution
Added validation that throws an informative error when
es_typeis explicitly provided buteffectsizeis not"boot":Implementation Details
The validation uses
match.call()to detect whether the user explicitly provided thees_typeparameter. This approach:es_typeChanges
estimate_contrasts.default()(10 lines)Backward Compatibility
es_typewitheffectsize = "boot"continues to workes_typecontinues to workes_typewith othereffectsizevalues will now error (this is intentional and addresses the issue)Fixes #575
Original prompt
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.