Add CatBoost documentation and test coverage for Explainer#924
Add CatBoost documentation and test coverage for Explainer#924aman-coder03 wants to merge 2 commits into
Conversation
jeongyoonlee
left a comment
There was a problem hiding this comment.
Could you reframe this PR as test + docs only?
The underlying bug doesn't reproduce on current CatBoost. CatBoostRegressor.feature_importances_ has been a class-level property since ~catboost 0.15 (2019), and check_conditions() fits the dummy model before the hasattr assertion (explainer.py:97-103) — so the original assertion already passes for CatBoost and default_importance() reads .feature_importances_ fine. Running the PR's own scenario on master (without this PR's code):
exp = Explainer(method="auto", model_tau=CatBoostRegressor(iterations=10, verbose=0), ...)
exp.get_importance() # passes: pd.Series, len == n_features, normalized to 1It passes, so test_explainer_auto_importance_catboost is green with and without the change, and the get_feature_importance() fallback is never reached for any of sklearn/lightgbm/xgboost/catboost (all satisfy feature_importances_ first) — it's dead code.
Please drop the _get_feature_importances helper and its fallback, keeping just the (genuinely additive) CatBoost-as-model_tau test and the docstring update. Also update the title/description away from "Bugfix" so the history doesn't record a fix for a non-bug.
Proposed Changes
this pr adds documentation and test coverage for using
CatBoostRegressoras themodel_tauestimator in theExplainerclass whenmethod="auto"what changed
model_taudocstring to mention catboost supportCatBoostRegressor, guarded withpytest.importorskip("catboost"), to verify thatExplainer(method="auto")works correctly and returns feature importances with the expected shapewhy
while investigating issue #826, i verified that current versions of catboost already expose the
feature_importances_attribute after fitting, soExplainer(method="auto")works without any implementation changesthis pr adds documentation and a regression test to ensure catboost support remains covered and continues to work as expected
closes #826
Types of changes
What types of changes does your code introduce to CausalML?
Put an
xin the boxes that applyChecklist
Put an
xin the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.Further comments
If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc. This PR template is adopted from appium.