-
-
Notifications
You must be signed in to change notification settings - Fork 40
marginaleffects 0.29.0 #1154
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
marginaleffects 0.29.0 #1154
Conversation
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.
Summary of Changes
Hello @vincentarelbundock, 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 updates the marginaleffects package to version 0.29.0, primarily focusing on internal refactoring and API adjustments. The changes aim to improve the robustness and consistency of how model attributes and prediction types are accessed and processed, particularly for Bayesian models and various prediction scenarios. The update includes a shift towards using a unified components() function for extracting model details and an adjustment in how prediction tests are conducted.
Highlights
- Internal API Refactoring for Model Components: The internal mechanism for accessing model components like
at_variablesand the basereg_modelhas been refactored. Instead of directly querying model attributes, the code now consistently uses themarginaleffects::components()function. This change enhances robustness and aligns with the package's internal API. - Streamlined Prediction Type Handling: The handling of
marginalmeansobjects within themodel_parametersmethod has been streamlined by removing a dedicated conditional block. This suggests a consolidation of logic or a shift in how these specific prediction types are processed, aiming for a more unified approach. - Updated Prediction Test Case: A key test case in
test-marginaleffects.Rwas updated to utilizemarginaleffects::predictions()instead ofmarginaleffects::avg_predictions(), and theallow_new_levels = TRUEargument was introduced. This indicates an update to how predictions are generated and tested, potentially reflecting new features or changes in themarginaleffectspackage's prediction API.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
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 updates the integration with the marginaleffects package. Several changes involve refactoring direct attribute access to use the marginaleffects::components() helper function, which is a good improvement for maintainability. However, I've identified two potential regressions. One is a misplaced suppressWarnings() call that will no longer suppress warnings as intended. The other is the removal of a code block responsible for handling marginalmeans objects, which could break functionality for that object type. The tests have also been updated to align with changes in the marginaleffects package.
| if (inherits(model, "marginalmeans")) { | ||
| attr(out, "coefficient_name") <- "Marginal Means" | ||
| } else if (inherits(model, "comparisons")) { | ||
| if (inherits(model, "comparisons")) { |
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.
The if block for marginalmeans objects has been removed. Since model_parameters.marginalmeans is an alias for this function (defined on line 90), this change will prevent the coefficient_name attribute from being set correctly for marginalmeans objects, which appears to be a regression in functionality. This block should be restored to ensure correct behavior.
if (inherits(model, "marginalmeans")) {
attr(out, "coefficient_name") <- "Marginal Means"
} else if (inherits(model, "comparisons")) {|
Let me integrate these changes into #1153, but I first want to finish easystats/bayestestR#733 |
|
|
||
| if (inherits(model, "marginalmeans")) { | ||
| attr(out, "coefficient_name") <- "Marginal Means" | ||
| } else if (inherits(model, "comparisons")) { |
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.
@vincentarelbundock Why did you remove this code?
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.
The package has not produced objecta of this class for over a year "marginalmeans"
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.
ah, ok!
| d <- insight::get_datagrid(m, by = "Days", include_random = TRUE) | ||
| x <- marginaleffects::avg_predictions(m, newdata = d, by = "Days") | ||
| x <- marginaleffects::predictions(m, newdata = d, allow_new_levels = TRUE) | ||
| out <- model_parameters(x) |
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.
@vincentarelbundock Why did you change the function call?
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.
Not sure, TBH. I think I had a problem that maybe wasn't a problem after all...
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 I wasn't actually sure what the intended output was, so...
@strengejacke
Unfortunately I won't be able to finish all the details on this, but I did want to put something together in case you have time/energy to look at this.
AFAICT, the
test-marginaleffects.Rall pass. But I'm not sure if there's anything else.