Skip to content
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

DOC convert to notebook style SVM C scaling example #21776

Merged
merged 8 commits into from Nov 29, 2022

Conversation

glemaitre
Copy link
Member

Follow-up after reviewing an example that was speeding up the notebook.
I just modify the example to be more notebook-style since we already had some sections in the summary docstring. Now, this is better in included.

In addition, I used the validation_curve instead of the grid-search since we don't have that many examples and this is a perfect analysis to use it. I also used pandas to make the plotting a little bit more intuitive. In general, we have many fewer indentation levels now.

@ogrisel
Copy link
Member

ogrisel commented Nov 24, 2021

check_regressors_train is segfaulting in one of the CI jobs. We need to apply the same workaround as done in #21702 and #21704 for check_classifiers_train while waiting for a proper fix upstream in either OpenBLAS (OpenMathLib/OpenBLAS#3453) or joblib (joblib/joblib#563).

@glemaitre
Copy link
Member Author

@thomasjpfan @lesteve it should not be a controversial DOC merge.

examples/svm/plot_svm_scale_c.py Outdated Show resolved Hide resolved
examples/svm/plot_svm_scale_c.py Outdated Show resolved Hide resolved
examples/svm/plot_svm_scale_c.py Outdated Show resolved Hide resolved
examples/svm/plot_svm_scale_c.py Outdated Show resolved Hide resolved
examples/svm/plot_svm_scale_c.py Outdated Show resolved Hide resolved
examples/svm/plot_svm_scale_c.py Outdated Show resolved Hide resolved
examples/svm/plot_svm_scale_c.py Outdated Show resolved Hide resolved
Copy link
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

Overall, this has the same message as the original example but in notebook style.

LGTM

@thomasjpfan thomasjpfan added the Waiting for Second Reviewer First reviewer is done, need a second one! label Nov 28, 2022
@glemaitre
Copy link
Member Author

Since this PR is hanging for one year out there and it only changes the style, I will merge it upon a single approval, as we do for trivial documentation changes.

Thanks @thomasjpfan

@glemaitre glemaitre merged commit 507ecad into scikit-learn:main Nov 29, 2022
@ArturoAmorQ
Copy link
Member

I was just making a review at the moment :')
Next time I'll remember to self-request it!

@glemaitre
Copy link
Member Author

@ArturoAmorQ do not hesitate to open a new PR if you have further improvements.
I will be happy to review it then.

glemaitre added a commit to glemaitre/scikit-learn that referenced this pull request Dec 21, 2022
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
glemaitre added a commit that referenced this pull request Dec 21, 2022
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
jjerphan pushed a commit to jjerphan/scikit-learn that referenced this pull request Jan 20, 2023
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
jjerphan pushed a commit to jjerphan/scikit-learn that referenced this pull request Jan 20, 2023
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation Waiting for Second Reviewer First reviewer is done, need a second one!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants