Skip to content

MAINT validate parameters in LogisticRegression and LogisiticRegressionCV #23565

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
Merged
merged 31 commits into from
Jun 27, 2022
Merged

MAINT validate parameters in LogisticRegression and LogisiticRegressionCV #23565

merged 31 commits into from
Jun 27, 2022

Conversation

Jitensid
Copy link
Contributor

@Jitensid Jitensid commented Jun 8, 2022 •
edited
Loading

Reference Issues/PRs

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

What does this implement/fix? Explain your changes.

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

Any other comments?

  1. 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.

@Jitensid 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
@Jitensid
Copy link
Contributor Author

Jitensid commented Jun 8, 2022 •
edited
Loading

Closed the Pull Request because of failure of tests.

@Jitensid Jitensid closed this Jun 8, 2022
@Jitensid Jitensid reopened this Jun 8, 2022
@Jitensid
Copy link
Contributor Author

Jitensid commented Jun 8, 2022

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

@Jitensid 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
jeremiedbb
jeremiedbb reviewed Jun 10, 2022
View reviewed changes
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.

Thanks for the PR @Jitensid. Here are some suggestions

@Jitensid 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 jeremiedbb added the Validation related to input validation label Jun 13, 2022
@Jitensid Jitensid requested a review from jeremiedbb June 15, 2022 08:57
@Jitensid Jitensid mentioned this pull request Jun 21, 2022
Closed
@jeremiedbb
Copy link
Member

@Jitensid I directly pushed some small fixes. Otherwise LGTM

jeremiedbb
jeremiedbb approved these changes Jun 24, 2022
View reviewed changes
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

@glemaitre glemaitre self-requested a review June 24, 2022 15:28
@glemaitre glemaitre changed the title [MRG] Logistic regression and Logistic Regression CV use _validate_params MAINT validate parameters in LogisticRegression and LogisiticRegressionCV Jun 24, 2022
@Jitensid
Copy link
Contributor Author

@glemaitre I have made changes as per your suggestions.

@glemaitre
Copy link
Member

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
glemaitre reviewed Jun 24, 2022
View reviewed changes
Copy link
Member

@glemaitre glemaitre left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM

@Jitensid
Copy link
Contributor Author

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

@glemaitre glemaitre merged commit 1f0c7e9 into scikit-learn:main Jun 27, 2022
@glemaitre
Copy link
Member

Thanks @Jitensid All good to be merged.

@Jitensid
Copy link
Contributor Author

Thanks @Jitensid All good to be merged.

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

ogrisel pushed a commit to ogrisel/scikit-learn that referenced this pull request Jul 11, 2022
@Jitensid @jeremiedbb
@glemaitre @ogrisel
MAINT validate parameters in LogisticRegression and LogisiticRegressi…
5b38fc2 
…onCV (scikit-learn#23565)

Co-authored-by: jeremie du boisberranger <jeremiedbb@yahoo.fr>
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@glemaitre glemaitre glemaitre left review comments

@BBloggsbott BBloggsbott BBloggsbott requested changes

@jeremiedbb jeremiedbb jeremiedbb approved these changes

Assignees
No one assigned
Labels
module:linear_model No Changelog Needed Validation related to input validation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Jitensid @jeremiedbb @glemaitre @BBloggsbott