-
Notifications
You must be signed in to change notification settings - Fork 2k
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
GH-6605 Fix feature interaction for CV model where tree has depth = 0 #15699
GH-6605 Fix feature interaction for CV model where tree has depth = 0 #15699
Conversation
@@ -43,6 +43,9 @@ public void mergeWith(FeatureInteractions featureInteractions) { | |||
} | |||
|
|||
public int maxDepth() { | |||
if (this.entrySet().size() == 0){ | |||
return 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's weird that actual problem is that FeatureInteractions are empty and all call has no meaning and we handle it in maxDepth funtion. It would be better to check that earlier and raise warning to the user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can be simulated this way also:
FeatureInteractions featureInteractions1 = new FeatureInteractions();
System.out.println("featureInteractions1 = " + Arrays.toString(featureInteractions1.getAsTable()));
in FeatureInteractionsTest
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @valenad1. Good points! I fixed the case there is no feature interaction for a model.
# feature interaction with cv model where tree depth = 0 | ||
my_cv_gbm <- h2o.getModel(my_gbm@model$cross_validation_models[[1]]$name) | ||
fi <-h2o.feature_interaction(model = my_cv_gbm) | ||
#print(fi == NaN) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would turn it to assert
expect_true(is.null(fi))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you!
The first part was fixed here: #15689.
But I found another bug: when the CV model tree has depth 0, it failed. So this PR fixed this bug.
Closes: #6605