MAINT validate parameters in LogisticRegression and LogisiticRegressionCV by Jitensid · Pull Request #23565 · scikit-learn/scikit-learn

Jitensid · 2022-06-08T09:29:47Z

Reference Issues/PRs

Logistic Regression and Logistic Regression CV uses _validate_parameters as part of #23462

What does this implement/fix? Explain your changes.

Logistic Regression and Logistic Regression CV has a new class attribute _parameter_constraints that defines the valid types and values for the parameters.
fit method first calls self._validate_params() method.
Removed the code for simple param validation for both the estimators.

Any other comments?

The test_check_solver_option test was failing because of assertion error due to mismatch of error messages. As of now I removed match=msg from it as I am not sure what to do about it.

This is my first contribution. If I missed anything let me know I will fix it as soon as possible.

…stic_regression_validate_params

Jitensid · 2022-06-08T12:11:35Z

Closed the Pull Request because of failure of tests.

…stic_regression_validate_params

Jitensid · 2022-06-08T18:52:32Z

Removed tests that were validating error messages for Logistic Regression and Logistic Regression CV estimators.

…tic regression

sklearn/linear_model/tests/test_logistic.py

…stic_regression_validate_params

jeremiedbb

Thanks for the PR @Jitensid. Here are some suggestions

sklearn/linear_model/_logistic.py

sklearn/linear_model/tests/test_logistic.py

…stic regression CV

…stic_regression_validate_params

…com/Jitensid/scikit-learn into logistic_regression_validate_params

jeremiedbb · 2022-06-24T14:38:26Z

@Jitensid I directly pushed some small fixes. Otherwise LGTM

jeremiedbb

LGTM

sklearn/linear_model/_logistic.py

…ssion and logsitic regression CV

Jitensid · 2022-06-24T15:46:09Z

@glemaitre I have made changes as per your suggestions.

glemaitre · 2022-06-24T15:47:03Z

I don't think that we need the test test_multinomial_validation:

@pytest.mark.parametrize("solver", ["lbfgs", "newton-cg", "sag", "saga"])
def test_multinomial_validation(solver):
    lr = LogisticRegression(C=-1, solver=solver, multi_class="multinomial")

    with pytest.raises(ValueError):
        lr.fit([[0, 1], [1, 0]], [0, 1])

C is negative and will be caught by the common parameter validation

glemaitre

Otherwise LGTM

Jitensid · 2022-06-24T15:56:10Z

Removed the test_multinomial_validation since C=-1 will cause ValueError and it is handled by param validation

glemaitre · 2022-06-27T07:20:15Z

Thanks @Jitensid All good to be merged.

Jitensid · 2022-06-27T07:44:22Z

Thanks @Jitensid All good to be merged.

Thank you @jeremiedbb @glemaitre for making my first open source contribution experience memorable.

…onCV (scikit-learn#23565) Co-authored-by: jeremie du boisberranger <jeremiedbb@yahoo.fr> Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>

Jitensid added 4 commits June 7, 2022 19:01

Added _parameter_constraints for Logistic Regression Class

d77f962

Removed code for simple param valiadation of Logistic Regression

8a2c0fd

Removed unncessary import

1b0db86

Merge branch 'main' of github.com:scikit-learn/scikit-learn into logi…

2c825f7

…stic_regression_validate_params

Jitensid changed the title ~~[MRG] Logistic regression and Logistic Regression use _validate_params~~ [MRG] Logistic regression and Logistic Regression CV use _validate_params Jun 8, 2022

github-actions bot added the module:linear_model label Jun 8, 2022

Jitensid closed this Jun 8, 2022

Jitensid added 3 commits June 8, 2022 23:56

Final Commit

f5655c3

Removed tests checking error msg by simple param validation

e6de713

Merge branch 'main' of github.com:scikit-learn/scikit-learn into logi…

1a80c96

…stic_regression_validate_params

Jitensid reopened this Jun 8, 2022

Jitensid changed the title ~~[MRG] Logistic regression and Logistic Regression CV use _validate_params~~ [WIP] Logistic regression and Logistic Regression CV use _validate_params Jun 8, 2022

Jitensid added 2 commits June 9, 2022 14:00

Changed value from 1e4 to 10000 of max_iter in ensemble test of logis…

7d2aba0

…tic regression

Added None in intercept_scaling parameter

a48b142

BBloggsbott suggested changes Jun 9, 2022

View reviewed changes

sklearn/linear_model/tests/test_logistic.py Show resolved Hide resolved

sklearn/linear_model/tests/test_logistic.py Show resolved Hide resolved

Merge branch 'main' of github.com:scikit-learn/scikit-learn into logi…

fdd64a4

…stic_regression_validate_params

jeremiedbb reviewed Jun 10, 2022

View reviewed changes

Jitensid added 3 commits June 11, 2022 15:13

Changed default value from None to 1.0 for intercept_scaling

93888f0

Removed unnecessary test for intercept_scaling parameter

7444477

changed param valdiations and used logistic regression params in logi…

ffef5ca

…stic regression CV

Jitensid changed the title ~~[WIP] Logistic regression and Logistic Regression CV use _validate_params~~ [MRG] Logistic regression and Logistic Regression CV use _validate_params Jun 11, 2022

jeremiedbb added the Validation related to input validation label Jun 13, 2022

Jitensid and others added 3 commits June 14, 2022 11:19

Merge branch 'main' into logistic_regression_validate_params

571226b

Merge branch 'main' of github.com:scikit-learn/scikit-learn into logi…

b233d9c

…stic_regression_validate_params

Merge branch 'main' into logistic_regression_validate_params

9022ed4

Jitensid requested a review from jeremiedbb June 15, 2022 08:57

Jitensid added 2 commits June 15, 2022 14:27

Merge branch 'main' into logistic_regression_validate_params

32a089a

Merge branch 'main' into logistic_regression_validate_params

69f48de

Jitensid added 2 commits June 16, 2022 20:03

Merge branch 'main' into logistic_regression_validate_params

031786d

Merge branch 'main' into logistic_regression_validate_params

355d09e

Jitensid mentioned this pull request Jun 21, 2022

Make all estimators use _validate_params #23462

Closed

Jitensid and others added 5 commits June 24, 2022 16:27

Merge branch 'main' into logistic_regression_validate_params

1f39bff

Merge branch 'main' of github.com:scikit-learn/scikit-learn into logi…

ae753cc

…stic_regression_validate_params

Modified bool constraint to boolean for validation_params

5004f4d

Merge branch 'main' of github.com:scikit-learn/scikit-learn into logi…

a03c415

…stic_regression_validate_params

Merge branch 'logistic_regression_validate_params' of https://github.…

c103101

…com/Jitensid/scikit-learn into logistic_regression_validate_params

jeremiedbb added the No Changelog Needed label Jun 24, 2022

jeremiedbb added 2 commits June 24, 2022 16:35

fixes

9f7a7c7

lint

1e5425f

jeremiedbb approved these changes Jun 24, 2022

View reviewed changes

glemaitre self-requested a review June 24, 2022 15:28

glemaitre changed the title ~~[MRG] Logistic regression and Logistic Regression CV use _validate_params~~ MAINT validate parameters in LogisticRegression and LogisiticRegressionCV Jun 24, 2022

nitpick

444c34c

glemaitre reviewed Jun 24, 2022

View reviewed changes

sklearn/linear_model/_logistic.py Outdated Show resolved Hide resolved

glemaitre reviewed Jun 24, 2022

View reviewed changes

sklearn/linear_model/_logistic.py Outdated Show resolved Hide resolved

Jitensid added 2 commits June 24, 2022 21:12

Using verbose constraint for the verbose param for the logistic regre…

70293c7

…ssion and logsitic regression CV

Using verbose constraint for the verbose param for the logistic regre…

fa52fc2

…ssion and logsitic regression CV

glemaitre reviewed Jun 24, 2022

View reviewed changes

Removed unnecessary test of test_multinomial_validation

71e6c97

glemaitre merged commit 1f0c7e9 into scikit-learn:main Jun 27, 2022

Uh oh!

Conversation

Jitensid commented Jun 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Any other comments?

Uh oh!

Jitensid commented Jun 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Jitensid commented Jun 8, 2022

Uh oh!

Uh oh!

Uh oh!

jeremiedbb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jeremiedbb commented Jun 24, 2022

Uh oh!

jeremiedbb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Jitensid commented Jun 24, 2022

Uh oh!

glemaitre commented Jun 24, 2022

Uh oh!

glemaitre left a comment

Choose a reason for hiding this comment

Uh oh!

Jitensid commented Jun 24, 2022

Uh oh!

glemaitre commented Jun 27, 2022

Uh oh!

Jitensid commented Jun 27, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Jitensid commented Jun 8, 2022 •

edited

Loading

Jitensid commented Jun 8, 2022 •

edited

Loading