Skip to content

Warnings in common tests about set_params #12652

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.

Sign up for GitHub

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

Jump to bottom
Open
amueller opened this issue Nov 22, 2018 · 9 comments
Open

Warnings in common tests about set_params #12652

amueller opened this issue Nov 22, 2018 · 9 comments
Assignees
@adrinjalali
Labels
module:test-suite everything related to our tests

Comments

@amueller
Copy link
Member

amueller commented Nov 22, 2018 •
edited
Loading

There's a bunch of "ValueError occurred during set_params.", introduced in #7760. This doesn't look good imho because it probably means we're violating API somewhere. We should investigate.
@amueller amueller added this to the 0.21 milestone Nov 22, 2018
@thomasjpfan
Copy link
Member

It looks like BaseSGD does validation when set_params is called:

scikit-learn/sklearn/linear_model/stochastic_gradient.py

Lines 103 to 106 in dbd28e7

def set_params(self, *args, **kwargs):
super(BaseSGD, self).set_params(*args, **kwargs)
self._validate_params(set_max_iter=False)
return self

For example, using set_params to set alpha=-np.inf in SGDClassifier will trigger a warning in test_check_estimator_clones.

@jnothman
Copy link
Member

jnothman commented Dec 2, 2018 via email

@amueller
Copy link
Member Author

So we should change the test? Or discontinue that practice? I would lean towards former but we tend to to the latter usually...

@thomasjpfan
Copy link
Member

thomasjpfan commented Dec 29, 2018 •
edited
Loading

I was always under the impression that parameter validation should be done at the beginning of fit. This sounds like something that should be standardized, maybe in a SLEP?

@jnothman
Copy link
Member

jnothman commented Jan 1, 2019

Yes, this is something that has become tacitly standardised over time, but we have some legacy behaviour.

It is hard to strictly maintain backwards compatibility while removing that legacy behaviour, but I'm okay to see what it looks like, or give up on strict compat.

@jnothman jnothman modified the milestones: 0.21, 0.22 Apr 24, 2019
@jnothman
Copy link
Member

I think this is inconsistent, but fairly innocuous... do we want to fix it?

@thomasjpfan
Copy link
Member

I would want to fix this to have a consistent API. The most conservative fix would be to issue a FutureWarning at first and then move the validation into fit.

Personally, I would prefer moving the validation into fit and consider this a bug fix.

@jnothman jnothman modified the milestones: 0.22, 0.23 Oct 31, 2019
@adrinjalali
Copy link
Member

take

@adrinjalali adrinjalali mentioned this issue Apr 17, 2020
Closed
@adrinjalali adrinjalali removed this from the 0.23 milestone Apr 21, 2020
@adrinjalali
Copy link
Member

removed the milestone, PR is now WIP.

@adrinjalali adrinjalali mentioned this issue May 25, 2020
Merged
@glemaitre glemaitre mentioned this issue Aug 5, 2021
Merged
@amueller amueller mentioned this issue Oct 23, 2021
Merged
@cmarmo cmarmo added the module:test-suite everything related to our tests label Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@adrinjalali adrinjalali

Labels
module:test-suite everything related to our tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[WIP] FIX remove validation from __init__ and set_param adrinjalali/scikit-learn
5 participants
@jnothman @amueller @cmarmo @adrinjalali @thomasjpfan