Skip to content

MAINT validate parameter in KernelPCA #24020

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

Merged
merged 7 commits into from
Jul 28, 2022

Conversation

kasmith11
Copy link
Contributor

Reference Issues/PRs

References #23462 as well as #22722

What does this implement/fix? Explain your changes.

Adds _parameter_constraints to sklearn/decomposition/_kernel_pca.py and removes KernelPCA from PARAM_VALIDATION_ESTIMATORS_TO_IGNORE in sklearn/tests/test_common.py

Any other comments?

@lucyleeow lucyleeow added the Validation related to input validation label Jul 27, 2022
@glemaitre glemaitre changed the title towards #23462 - Kernel PCA MAINT validate parameter in KernelPCA Jul 27, 2022
@glemaitre
Copy link
Member

The CIs are failing. You need to check the reason. Be aware that you will most probably need to edit the test file to remove some of the test that are covered now by the common test enabled by the parameter validation framework.

@jeremiedbb
Copy link
Member

Thanks for the PR @kasmith11. One of the reason you had failing tests was because the parameter description of kernel in the docstring was incorrect. It also accepts a callable. I pushed a fix for that, and also fixed the bounds of a couple of constraints. I also removed the outdated validation tests instead of commenting them. Should be good now.

Copy link
Member

@jeremiedbb jeremiedbb left a comment

Choose a reason for hiding this comment

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

LGTM

@jeremiedbb jeremiedbb merged commit 3312bc2 into scikit-learn:main Jul 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants