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

Add SHAP calculation to GBT regression #1399

Merged
merged 64 commits into from
Oct 27, 2023

Conversation

ahuber21
Copy link
Contributor

Pendant to oneapi-src/oneDAL#2460. This PR enables to new SHAP value functionality in scikit-learn-intelex.

The API is going to be similar to XGBoosts predict() method

xgb_model = xgb.XGBRegressor(max_depth=6, n_estimators=100, random_state=3)
xgb_model.fit(X_train, y_train)
booster = xgb_model.get_booster()

d4p_model = daal4py.mb.convert_model(booster)
d4p_model.predict(X_test, pred_contributions=True)  # new: pred_contributions
d4p_model.predict(X_test, pred_interactions=True)  # new: pred_interactions

generator/gen_daal4py.py Outdated Show resolved Hide resolved
src/gbt_convertors-recursive-append.pyx Outdated Show resolved Hide resolved
@ahuber21 ahuber21 force-pushed the dev/ahuber/gbt-shap branch 2 times, most recently from 336cb94 to 4a6a507 Compare September 27, 2023 16:25
@ahuber21 ahuber21 marked this pull request as ready for review September 28, 2023 10:13
@ahuber21
Copy link
Contributor Author

@ahuber21
Copy link
Contributor Author

@napetrov
Copy link
Contributor

i would also add example and documentation explaining new API

@ahuber21
Copy link
Contributor Author

i would also add example and documentation explaining new API

Added examples/daal4py/model_builders_xgboost_shap.py - where should I add the documentation?

@napetrov
Copy link
Contributor

i would also add example and documentation explaining new API

Added examples/daal4py/model_builders_xgboost_shap.py - where should I add the documentation?

https://github.com/intel/scikit-learn-intelex/blob/2c4dbb0593cbf3e5eff55a10cbc5fed1685b1505/doc/daal4py/model-builders.rst

@ahuber21
Copy link
Contributor Author

@napetrov done - updated the RST

@napetrov
Copy link
Contributor

i would also add example and documentation explaining new API

Added examples/daal4py/model_builders_xgboost_shap.py - where should I add the documentation?

Should we consider adding MB examples to separate directory? at same level as scikit/daal4py? @razdoburdin - might be you can share thoughts as well

The problem with keeping in daal4py examples is that it's hard to find, while in case of modelbuilders separate folder we can put all MB related examples in same place.

I think that majority of users are not overlapping between MB use and daal4py use

@razdoburdin
Copy link
Contributor

Should we consider adding MB examples to separate directory? at same level as scikit/daal4py? @razdoburdin - might be you can share thoughts as well

That is a good idea. In my mind the structure of examples should reproduce the structure of the python imports. Currently we are not close to this structure. But if we want to move in this direction, we should place all the model builders related examples in examples/daal4py/mb folder.

@ahuber21
Copy link
Contributor Author

@napetrov @razdoburdin I have created the model_builders subdirectory. Did I catch all the examples?

@razdoburdin
Copy link
Contributor

@napetrov @razdoburdin I have created the model_builders subdirectory. Did I catch all the examples?

Yes, they are all in place.
But I think we should move the subdirectory inside daal4py. @ahuber21 @napetrov, how do you think?

@napetrov
Copy link
Contributor

model_builders

i would keep at same level as daal4py/scikit . Although technically this is part of daal4py but from usage scenario it ether of those.

The only question is this should be model_builders but ether "Model builders" or MB? The point - there is no places there we have model_builders format used.

@ahuber21
Copy link
Contributor Author

Don't have a strong opinion here @napetrov. Do you want me to rename the directory to mb?

@napetrov
Copy link
Contributor

Don't have a strong opinion here @napetrov. Do you want me to rename the directory to mb?

I'm not sure on corrects one - but version with underline doesn't match other usages. mb would be consistent with module name

requirements-test.txt Outdated Show resolved Hide resolved
@ahuber21 ahuber21 merged commit 6d95372 into intel:master Oct 27, 2023
2 of 18 checks passed
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.

6 participants