Skip to content

FEA add ValidationCurveDisplay in model_selection module #25120

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 52 commits into from
Jun 14, 2023
Merged

FEA add ValidationCurveDisplay in model_selection module #25120

merged 52 commits into from
Jun 14, 2023

Conversation

glemaitre
Copy link
Member

@glemaitre glemaitre commented Dec 6, 2022 •
edited
Loading

Add ValidationCurveDisplay to the model_selection module.

This is the missing curve for the module.

@glemaitre glemaitre marked this pull request as draft December 6, 2022 12:52
@glemaitre glemaitre marked this pull request as ready for review December 6, 2022 13:42
@glemaitre
Copy link
Member Author

ping @ArturoAmorQ since this is something that we probably used in the MOOC.

@ArturoAmorQ
Copy link
Member

Don't forget to add the ValidationCurveDisplay to the Display Objects section of the Visualizations module in the User Guide ;)

ArturoAmorQ
ArturoAmorQ reviewed Feb 1, 2023
View reviewed changes
Copy link
Member

@ArturoAmorQ ArturoAmorQ 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 @glemaitre, this will certainly be a nice addition!
Here are a couple of comments.

@glemaitre glemaitre force-pushed the validation_curve_display branch from ebec90b to c1d496c Compare February 2, 2023 09:24
@jeremiedbb jeremiedbb added this to the 1.3 milestone Feb 28, 2023
glemaitre
glemaitre commented Mar 23, 2023
View reviewed changes
doc/modules/learning_curve.rst
@@ -71,7 +71,7 @@ The function :func:`validation_curve` can help in this case::
>>> import numpy as np
>>> from sklearn.model_selection import validation_curve
>>> from sklearn.datasets import load_iris
>>> from sklearn.linear_model import Ridge
>>> from sklearn.svm import SVC
Copy link
Member Author

Choose a reason for hiding this comment

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

The problem here is that we used a regressor for a classification problem.

jeremiedbb
jeremiedbb reviewed Mar 27, 2023
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.

A quick first pass.

glemaitre and others added 3 commits March 28, 2023 17:35
@glemaitre @jeremiedbb
Update sklearn/model_selection/_plot.py
07c10a0 
Co-authored-by: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com>
@glemaitre
Merge branch 'main' into validation_curve_display
e322af0
@glemaitre
typo
7c223bf
jeremiedbb
jeremiedbb reviewed Jun 12, 2023
View reviewed changes
@lorentzenchr
Copy link
Member

exploit_incremental_learning should be better documented. It is not clear to me which estimator supports this.

@jeremiedbb
Copy link
Member

jeremiedbb commented Jun 12, 2023 •
edited
Loading

exploit_incremental_learning should be better documented. It is not clear to me which estimator supports this.

This is a parameter of learning_curve, but this PR is about adding a display for validation_curve. I think a dedicated issue/Pr is more appropriate.

ogrisel
ogrisel reviewed Jun 13, 2023
View reviewed changes
Copy link
Member

@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

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

Another pass. Otherwise LGTM.

ogrisel
ogrisel reviewed Jun 13, 2023
View reviewed changes
Copy link
Member

@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

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

More feedback:

glemaitre and others added 6 commits June 14, 2023 11:46
@glemaitre @ogrisel @jeremiedbb
Apply suggestions from code review
81f5c2a 
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Co-authored-by: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com>
@glemaitre @ogrisel
Apply suggestions from code review
ffc7786 
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
@glemaitre @ogrisel @jeremiedbb
Apply suggestions from code review
7a63a78 
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Co-authored-by: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com>
@glemaitre @jeremiedbb
Apply suggestions from code review
fe4077f 
Co-authored-by: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com>
@glemaitre
move figure
0cf87b1
@glemaitre
iter
10a25cf
@glemaitre
Copy link
Member Author

This is a parameter of learning_curve, but this PR is about adding a display for validation_curve. I think a dedicated issue/Pr is more appropriate.

Fine doing this in another PR. We also need to improve the learning_curve documentation then because it is just a copy paste from this documentation.

jeremiedbb
jeremiedbb reviewed Jun 14, 2023
View reviewed changes
jeremiedbb
jeremiedbb reviewed Jun 14, 2023
View reviewed changes
doc/whats_new/v1.3.rst Outdated
Comment on lines 58 to 59
train and test curves to be consistent with
:class:`model_selection.ValidationCurveDisplay`. You can set `score_type="test"` to
Copy link
Member

Choose a reason for hiding this comment

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

The motivation is more that it's a better default behavior for the typical use cases, rather than consistency

ogrisel
ogrisel approved these changes Jun 14, 2023
View reviewed changes
Copy link
Member

@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

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

Once the above suggestions and the following are addressed, LGTM!

glemaitre and others added 3 commits June 14, 2023 17:50
@glemaitre @ogrisel
Update doc/whats_new/v1.3.rst
afa6264 
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
@glemaitre @ogrisel
Apply suggestions from code review
194f085 
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
@glemaitre
iter
c806b22
@jeremiedbb jeremiedbb enabled auto-merge (squash) June 14, 2023 16:26
@jeremiedbb jeremiedbb merged commit 8cac52f into scikit-learn:main Jun 14, 2023
@ogrisel
Copy link
Member

ogrisel commented Jun 15, 2023

Thanks @glemaitre and @jeremiedbb!

REDVM pushed a commit to REDVM/scikit-learn that referenced this pull request Nov 16, 2023
@glemaitre @jeremiedbb
@ogrisel @REDVM
FEA add ValidationCurveDisplay in model_selection module (scikit-lear…
2d602f8 
…n#25120)

Co-authored-by: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com>
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeremiedbb jeremiedbb jeremiedbb approved these changes

@ArturoAmorQ ArturoAmorQ ArturoAmorQ left review comments

@ogrisel ogrisel ogrisel approved these changes

Assignees
No one assigned
Labels
module:model_selection
Projects
None yet
Milestone
1.3
Development

Successfully merging this pull request may close these issues.
5 participants
@glemaitre @ArturoAmorQ @lorentzenchr @jeremiedbb @ogrisel