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

Expose raw fitted GLM model in report #41

Merged
merged 5 commits into from
Jan 18, 2024
Merged

Conversation

ablaom
Copy link
Member

@ablaom ablaom commented Jan 16, 2024

Addresses: #40

This PR add :raw_glm_model to the list of user-specifiable report items (controlled by the report_keys hyperparameter.

The default keys now includes the new :raw_glm_model key (and all other keys, as presently the case). This means that, by default, the report points to the training data, because raw GLM models do so. I'd suggest we tag this change as breaking for this reason, although technically, adding a keys, doesn't break the API.

Copy link

codecov bot commented Jan 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (924d0e9) 96.72% compared to head (12c9ff5) 96.79%.

❗ Current head 12c9ff5 differs from pull request most recent head 6bba7aa. Consider uploading reports for the commit 6bba7aa to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #41      +/-   ##
==========================================
+ Coverage   96.72%   96.79%   +0.07%     
==========================================
  Files           1        1              
  Lines         183      187       +4     
==========================================
+ Hits          177      181       +4     
  Misses          6        6              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@rikhuijzer rikhuijzer left a comment

Choose a reason for hiding this comment

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

As a nitpick comment: I think it would also make sense to not have raw_glm_model enabled by default, but I don't have a strong opinion on this. There are good arguments for both sides.

Apart from that, everything looks fine to me. Thanks for implementing this Anthony!

@ablaom
Copy link
Member Author

ablaom commented Jan 16, 2024

I think it would also make sense to not have raw_glm_model enabled by default

Done.

@OkonSamuel
Copy link
Member

Addresses: #40

This PR add :raw_glm_model to the list of user-specifiable report items (controlled by the report_keys hyperparameter.

The default keys now includes the new :raw_glm_model key (and all other keys, as presently the case). This means that, by default, the report points to the training data, because raw GLM models do so. I'd suggest we tag this change as breaking for this reason, although technically, adding a keys, doesn't break the API.

Maybe we can change the name of :raw_glm_model to glm_object or something else. It's just a suggestion though.

@ablaom
Copy link
Member Author

ablaom commented Jan 16, 2024

How about glm_model?

@ablaom ablaom merged commit 7f48db6 into master Jan 18, 2024
2 checks passed
@rikhuijzer
Copy link
Member

Thanks @ablaom and @OkonSamuel!

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.

3 participants