dswah / pyGAM

[HELP REQUESTED] Generalized Additive Models in Python
https://pygam.readthedocs.io
Apache License 2.0
857 stars 157 forks source link

Added "terms" and "response" type #275

Closed shyamcody closed 4 years ago

shyamcody commented 4 years ago

Added "terms" and "response" type in predict method of the GAM class in similarity with mgcv package in r. The default type parameter is "response" which provides the prediction value. When type parameter is set to "terms", term wise contribution in the linear part of the prediction for each feature is provided.

codecov[bot] commented 4 years ago

Codecov Report

Merging #275 into master will decrease coverage by 0.08%. The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #275      +/-   ##
==========================================
- Coverage   95.21%   95.13%   -0.09%     
==========================================
  Files          22       22              
  Lines        3178     3183       +5     
==========================================
+ Hits         3026     3028       +2     
- Misses        152      155       +3     
Impacted Files Coverage Δ
pygam/pygam.py 94.45% <50.00%> (-0.35%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update b57b4cf...5369173. Read the comment docs.

shyamcody commented 4 years ago

@dswah can you provide a review?

dswah commented 4 years ago

@shyamcody this looks pretty cool.

A couple of questions:

  1. Other than compatibility with mgcv, what is the use-case for this change? I guess with this change it is easier to build partial dependence plots? Do you think we are adding a new feature or are we just adding another way to do the same thing?
  2. Can we change the name of the keyword type_ ? I know it seems a little silly, but the underscore bothers me. I suppose you intentionally avoided type because of the python built-in... Do you have any other ideas? Perhaps simply output?

I think we need 2 important changes though!

  1. Currently this PR makes no check for data quality. This is important, but easy to fix. instead of calling self._linear_predictor(...) in the loop, you could call self.partial_dependence(X=X, term=term).
  2. Please add a test for this PR, to ensure that the shapes of the outputs are as you expected. I can help you with this if you need.
shyamcody commented 4 years ago

hi, @dswah thanks for the reply. (1) The usability for me is compatibility only; i.e. this is not an addition. In a way, this is another way to do the same thing. But a lot of users are shifting from mgcv to pygam, so maybe its good to add it. (2) I can definitely change the type_ into output and add the other relevant changes.