-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
API Adds feature_names_in_ to kernel_approximation #20841
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
API Adds feature_names_in_ to kernel_approximation #20841
Conversation
@@ -3722,6 +3722,7 @@ def check_dataframe_column_names_consistency(name, estimator_orig): | |||
set_random_state(estimator) | |||
|
|||
X_orig = rng.normal(size=(150, 8)) | |||
X_orig -= X_orig.min() + 0.5 |
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.
Added this because the original fails for SkewedChi2Sampler.transform
with skewedness=1.0
:
scikit-learn/sklearn/kernel_approximation.py
Lines 475 to 476 in d61662b
if (X <= -self.skewedness).any(): | |
raise ValueError("X may not contain entries smaller than -skewedness.") |
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 stumbled upon the same issue when trying to tackle this module. I am fine with your solution because I could not find a better idea...
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.
Could you add a comment for that line? And maybe use X_orig += X_orig.abs().max() + 0.5
to be sure it's positive.
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 added a comment. I left the -= .min()
pattern because it is used several times elsewhere in the test file.
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
@@ -3722,6 +3722,7 @@ def check_dataframe_column_names_consistency(name, estimator_orig): | |||
set_random_state(estimator) | |||
|
|||
X_orig = rng.normal(size=(150, 8)) | |||
X_orig -= X_orig.min() + 0.5 |
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.
Could you add a comment for that line? And maybe use X_orig += X_orig.abs().max() + 0.5
to be sure it's positive.
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Reference Issues/PRs
Follow up to #18010