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 validations for explainable model arguments in MimicExplainer #354

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

gaugup
Copy link
Collaborator

@gaugup gaugup commented Dec 15, 2020

  • It seems that there is no validations for explainable model parameters. This may cause cryptic Exceptions from explainable model classes. So catching these errors earlier.
  • Also refactoring some code around setting of parameters for explainable_model_args.

Signed-off-by: Gaurav Gupta [email protected]

@@ -288,7 +289,6 @@ def __init__(self, model, initialization_examples, explainable_model, explainabl
# Index the categorical string columns for training data
self._column_indexer = initialization_examples.string_index(columns=categorical_features)
self._one_hot_encoder = None
Copy link
Collaborator

Choose a reason for hiding this comment

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

why was this removed?

             explainable_model_args[LightGBMParams.CATEGORICAL_FEATURE] = categorical_features 

Copy link
Collaborator

Choose a reason for hiding this comment

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

oh I guess you moved it to line 347, I guess that's ok, my only slight concern is now we are doing the same checks in multiple places:

is_tree_model = explainable_model.explainable_model_type == ExplainableModelType.TREE_EXPLAINABLE_MODEL_TYPE
        if is_tree_model and self._supports_categoricals(explainable_model):

but it's not expensive so I think it's ok

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants