Skip to content

Conversation

@vincentarelbundock
Copy link
Contributor

@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.R all pass. But I'm not sure if there's anything else.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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_variables and the base reg_model has been refactored. Instead of directly querying model attributes, the code now consistently uses the marginaleffects::components() function. This change enhances robustness and aligns with the package's internal API.
  • Streamlined Prediction Type Handling: The handling of marginalmeans objects within the model_parameters method 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.R was updated to utilize marginaleffects::predictions() instead of marginaleffects::avg_predictions(), and the allow_new_levels = TRUE argument was introduced. This indicates an update to how predictions are generated and tested, potentially reflecting new features or changes in the marginaleffects package'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

  1. 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.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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")) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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")) {

@strengejacke
Copy link
Member

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")) {
Copy link
Member

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?

Copy link
Contributor Author

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"

Copy link
Member

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)
Copy link
Member

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?

Copy link
Contributor Author

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...

Copy link
Contributor Author

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...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Won't fix 🚫 This will not be worked on

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants