-
-
Notifications
You must be signed in to change notification settings - Fork 26k
ENH Checks n_features_in_ in covariance #19341
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
ENH Checks n_features_in_ in covariance #19341
Conversation
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
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.
Do we want to validate data also for score
method of the EmpiricalCovariance
class?
@@ -163,6 +163,7 @@ def decision_function(self, X): | |||
compatibility with other outlier detection algorithms. | |||
""" | |||
check_is_fitted(self) | |||
X = self._validate_data(X, reset=False) |
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.
Shall make the validation in score_samples
instead since it is call below?
Right now, we don't have any check in this method.
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.
Yes. In this specific case and since predict
, decision_function
, score
all call score_samples
, let's move the validation to score_samples
.
The common test does not currently cover score. We most likely need to go through all the modules to add |
Edit: I looked at the wrong |
Shouldn't there be a |
I think Looking at the code base, adding |
Updated PR with adding |
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 - again.
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 @thomasjpfan |
Reference Issues/PRs
Related to #19333