estimate_contrasts() problem with p-value adjustments#593
estimate_contrasts() problem with p-value adjustments#593strengejacke merged 10 commits intomainfrom
estimate_contrasts() problem with p-value adjustments#593Conversation
Summary of ChangesHello @strengejacke, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses an issue with p-value adjustments, particularly for the Tukey method, by refining how statistical values are identified and utilized. The changes enhance the reliability of the adjustment process by directly querying available statistic columns, ensuring correct application of the adjustment without relying on external extraction methods. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses an issue with p-value adjustments in estimate_contrasts() by changing how the test statistic is retrieved. The change involves removing the early retrieval of the statistic from the model object and instead extracting it from the params data frame within the Tukey adjustment logic. This makes the process more robust, especially for model types where insight::get_statistic() might not work as expected. The overall change is a good improvement. I've added one suggestion to improve the readability of the code that finds the statistic column.
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request fixes an issue with Tukey p-value adjustments in estimate_contrasts(). The core logic in R/p_adjust.R has been improved by sourcing the test statistic from the results data frame and adding validation checks for Tukey's HSD conditions. The p-value adjustment call has also been relocated to a more suitable position in R/get_marginalcontrasts.R. New tests have been added to verify the fix. Overall, the changes are robust and well-implemented. I have one suggestion regarding a new test that is conditionally skipped on recent R versions, which could pose a maintenance risk. The remaining changes are stylistic and improve code readability.
There was a problem hiding this comment.
Pull request overview
This pull request fixes issue #592 where estimate_contrasts() was producing incorrect p-values when using Tukey adjustment. The p-values were being scrambled - they had the correct values but were assigned to the wrong comparisons.
Changes:
- Fixed p-value calculation in Tukey adjustment by extracting test statistics from the contrasts dataframe instead of the model object
- Moved p-value adjustment to occur after attributes are set, ensuring all necessary metadata is available
- Added validation checks to prevent Tukey adjustment when requirements aren't met (minimum 2 groups, positive degrees of freedom)
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| R/p_adjust.R | Fixed statistic extraction to use params dataframe columns instead of model object; added validation for Tukey adjustment requirements |
| R/get_marginalcontrasts.R | Moved p-value adjustment call to after attribute setting; minor formatting improvements |
| tests/testthat/test-estimate_contrasts.R | Added regression tests for the Tukey adjustment fix; extensive formatting improvements for readability |
| NEWS.md | Documented the bug fix |
| DESCRIPTION | Version bump to 0.13.1.2 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Fixes #592