Skip to content
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

interpret support group effects #721

Merged
merged 8 commits into from
Sep 18, 2023

Conversation

GStechschulte
Copy link
Collaborator

@GStechschulte GStechschulte commented Sep 15, 2023

This PR resolves the KeyError in PR #716. This error was the result of no default values being computed for model terms specified as group level effects. This is because group level effects do not have a components attribute, and thus never satisfied the logic in the set_default_values() function of ../interpret/utils.py.

Now, to compute default values, all unique common and group specific effects variable names are identified by accessing the components and factor attributes and are put into a list. From there, the data type is determined by identifying the data types used to fit the model which is then used to determine the function call (mean or mode for numeric and categorical dtypes, respectively).

Additionally, with group specific effects, when the user (or default values are computed) passes values, it is possible these values are new unseen data. Thus, I leverage PR #693 and add the arg sample_new_groups to the functions predictions, comparisons, and slopes (and the associated plotting functions) to allow plotting of new groups.

For a working example, view my last comment in PR #716.

To do:

  • Add tests for new a model with group specific effects

@GStechschulte GStechschulte marked this pull request as ready for review September 15, 2023 08:19
@tomicapretto
Copy link
Collaborator

I would like to ask two things:

  • Don't include anything related to the PR [715] Adding Mr P docs #716. As far as it is, this PR would add the notebook. I think it's because the branch was created from the branch in that PR. It should be created from the main branch.
  • Could you add a test for the new supported cases? One reason why we didn't catch it before is that it was untested 😅

@codecov-commenter
Copy link

codecov-commenter commented Sep 15, 2023

Codecov Report

Merging #721 (abed003) into main (16b005b) will increase coverage by 0.05%.
The diff coverage is 95.00%.

@@            Coverage Diff             @@
##             main     #721      +/-   ##
==========================================
+ Coverage   89.50%   89.56%   +0.05%     
==========================================
  Files          44       44              
  Lines        3526     3525       -1     
==========================================
+ Hits         3156     3157       +1     
+ Misses        370      368       -2     
Files Changed Coverage Δ
bambi/interpret/plotting.py 84.96% <ø> (ø)
bambi/interpret/utils.py 87.68% <93.75%> (+0.92%) ⬆️
bambi/interpret/effects.py 89.24% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@GStechschulte
Copy link
Collaborator Author

Yes, you are right. I checked out this branch from Nathaniel's branch. I have removed everything from that branch now. As for tests go, I have added a model with common and group effects. I test each plotting function in interpret for new unseen data and non-new observed data.

Copy link
Collaborator

@tomicapretto tomicapretto left a comment

Choose a reason for hiding this comment

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

Looks great! You can hit the green button! (squash and merge)

@GStechschulte GStechschulte merged commit ab54b99 into bambinos:main Sep 18, 2023
@GStechschulte GStechschulte deleted the interpret-group-effects branch September 28, 2023 07:06
GStechschulte added a commit to GStechschulte/bambi that referenced this pull request Oct 3, 2023
* [715] Adding Mr P docs

Signed-off-by: Nathaniel <[email protected]>

* [715] updating black formatting and suppressing numpyro messages

Signed-off-by: Nathaniel <[email protected]>

* [715] adding plot_comparisons checks on basic model

Signed-off-by: Nathaniel <[email protected]>

* remove Mr.P example and metadata

* support group-specific-effects default values and sampling from new groups

* use set with in for conditional statement

* remove commits related to PR bambinos#715

* add group specific effects for interpret plotting functions

---------

Signed-off-by: Nathaniel <[email protected]>
Co-authored-by: Nathaniel <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants