-
-
Notifications
You must be signed in to change notification settings - Fork 26.5k
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
lorentzenchr
left a comment
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
glemaitre
left a comment
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?
| 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 |
lorentzenchr
left a comment
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.
glemaitre
left a comment
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