-
-
Notifications
You must be signed in to change notification settings - Fork 104
Harmonizing argument names for check_* functions #886
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
Conversation
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 focuses on improving code consistency and readability by harmonizing the primary argument name across the Highlights
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
|
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.
Code Review
This pull request improves the consistency of the check_model() function family by renaming the primary argument from x to model. The changes are applied correctly across the function definitions, documentation, and internal function calls within R/check_model.R. This is a valuable refactoring that enhances code readability and predictability for users.
To further improve consistency across the package, as suggested by the pull request title "Harmonizing argument names for check_* functions", I recommend extending this harmonization to other check_* functions that still use x or object as the name for the model argument. For example, functions like check_collinearity(), check_normality(), and simulate_residuals() still use x, while check_predictions() uses object. Aligning these to use model would create a more uniform API across the entire package.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #886 +/- ##
==========================================
+ Coverage 61.90% 62.61% +0.71%
==========================================
Files 89 93 +4
Lines 6641 8218 +1577
==========================================
+ Hits 4111 5146 +1035
- Misses 2530 3072 +542 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@gemini-code-assist review |
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.
Code Review
This pull request improves consistency by renaming the primary argument to model in check_convergence, check_model, and check_predictions. This is a good step towards a more readable and maintainable API.
To make the harmonization more complete, you could consider extending this change to other check_* functions in the package (like check_collinearity, check_normality, etc.) that still use x as the argument for the model object.
I've added one specific comment regarding an internal function where the parameter name could also be updated for consistency within the file.
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 addresses easystats consistency by harmonizing the primary argument name for several check_* functions to use model instead of x/object (Fixes #882).
Changes:
- Renamed the first argument of
check_model(),check_predictions(), andcheck_convergence()tomodel. - Updated S3 method signatures to align with the new
modelargument naming. - Updated
.Rddocumentation to reflect the renamed arguments.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
R/check_predictions.R |
Renames object → model in exported generic and methods. |
R/check_model.R |
Renames x → model in exported generic and methods. |
R/check_convergence.R |
Renames x → model in exported function signature. |
man/check_predictions.Rd |
Updates usage/arguments from object → model. |
man/check_model.Rd |
Updates usage/arguments from x → model. |
man/check_convergence.Rd |
Updates usage/arguments from x → model. |
Comments suppressed due to low confidence (1)
man/check_convergence.Rd:16
- The documentation says
...is "Currently not used", but the implementation forwards...toinsight::is_converged(). Please update the...argument description to reflect that it is passed through (or stop forwarding it if it truly shouldn’t be used).
check_convergence(model, tolerance = 0.001, ...)
}
\arguments{
\item{model}{A \code{merMod} or \code{glmmTMB}-object.}
\item{tolerance}{Indicates up to which value the convergence result is
accepted. The smaller \code{tolerance} is, the stricter the test will be.}
\item{...}{Currently not used.}
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@profandyfield In your book, do you name the first argument in |
|
I've just done a search for |
|
Great, thanks! |
|
@gemini-code-assist review |
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.
Code Review
This pull request harmonizes argument names across several check_* functions, which improves consistency. I've found a few critical issues with the implementation of the backward compatibility for deprecated arguments, which could lead to errors when users call the functions with the old argument names. I've also suggested adding model validation checks for increased robustness in a few places. Please see my detailed comments.
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
Copilot reviewed 9 out of 9 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Fixes #882