8000 DOC Rework Importance of Feature Scaling example by ArturoAmorQ · Pull Request #25012 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

DOC Rework Importance of Feature Scaling example #25012

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 35 commits into from
Jan 19, 2023
Merged

DOC Rework Importance of Feature Scaling example #25012

merged 35 commits into from
Jan 19, 2023

Conversation

ArturoAmorQ
Copy link
Member

Reference Issues/PRs

Fixes #12282.

What does this implement/fix? Explain your changes.

This example can benefit from a "tutorialization". In particular, this PR adds a section regarding how nearest neighbors is sensitive to scaling.

Any other comments?

Side effect: Implements notebook style as intended in #22406.

@glemaitre
Copy link
Member

Weird that the CIs did not start. I merged main in the branch to trigger them.

lorentzenchr
lorentzenchr reviewed Dec 8, 2022
View reviewed changes
Copy link
Member
@lorentzenchr lorentzenchr left a comment

Choose a reason for hiding this comment

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

My 5 cent.

ArturoAmorQ and others added 2 commits December 9, 2022 11:30
@ArturoAmorQ @lorentzenchr
Update examples/preprocessing/plot_scaling_importance.py
0491e92 
Co-authored-by: Christian Lorentzen <lorentzen.ch@gmail.com>
Merge branch 'scaling_importance' of github.com:ArturoAmorQ/scikit-le…
41d98de 
…arn into scaling_importance
@glemaitre glemaitre self-assigned this Dec 9, 2022
@glemaitre glemaitre removed their assignment Dec 9, 2022
@ArturoAmorQ
Copy link
Member Author
ArturoAmorQ 10000 commented Dec 12, 2022

I think all of your comments have been addressed, @glemaitre and @lorentzenchr.
Please let me know what do you think of my proposed solutions.

@glemaitre
Copy link
Member

I will have to check the rendering but I think that the proposal is already an improvement.

lorentzenchr
lorentzenchr approved these changes Dec 15, 2022
View reviewed changes
Copy link
Member
@lorentzenchr lorentzenchr left a comment

Choose a reason for hiding this comment

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

LGTM, some nitpicks.

examples/preprocessing/plot_scaling_importance.py
)
scaled_X_train = scaler.fit_transform(X_train)

Copy link
Member

Choose a reason for hiding this comment

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

Optional: We could show the mean value of each feature, or min and max.

examples/preprocessing/plot_scaling_importance.py Outdated

# %%
# The need for regularization is higher (lower values of `C 6D47 `) for the data
# that was not scaled before applying PCA. From the plot we can confirm that
Copy link
Member

Choose a reason for hiding this comment

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

Which plot? Is it over- or underfitting?

Copy link
Member Author

Choose a reason for hiding this comment

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

By plotting the validation curves I realized that the training and test accuracy overlap too much to make a proper statement about over- or underfitting for the scenario with no standardization.

output1
output2

I think that it is better to avoid mentioning over-/underfitting to keep the example as simple as possible.

glemaitre
glemaitre approved these changes Jan 17, 2023
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.

Only nitpicks. Otherwise LGTM.

@ArturoAmorQ @glemaitre
Apply suggestions from code review
ff8ba3b 
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
@glemaitre
Copy link
Member

I certainly broke the linter with my suggestion. Sorry @ArturoAmorQ

@glemaitre glemaitre merged commit 4b55dee into scikit-learn:main Jan 19, 2023
@ArturoAmorQ ArturoAmorQ deleted the scaling_importance branch January 19, 2023 16:12
jjerphan pushed a commit to jjerphan/scikit-learn that referenced this pull request Jan 20, 2023
@ArturoAmorQ @glemaitre
@lorentzenchr @jjerphan
DOC Rework Importance of Feature Scaling example (scikit-learn#25012)
4575b54 
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-authored-by: Christian Lorentzen <lorentzen.ch@gmail.com>
jjerphan pushed a commit to jjerphan/scikit-learn that referenced this pull request Jan 20, 2023
@ArturoAmorQ @glemaitre
@lorentzenchr @jjerphan
DOC Rework Importance of Feature Scaling example (scikit-learn#25012)
23fe0c9 
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-authored-by: Christian Lorentzen <lorentzen.ch@gmail.com>
jjerphan pushed a commit to jjerphan/scikit-learn that referenced this pull request Jan 23, 2023
@ArturoAmorQ @glemaitre
@lorentzenchr @jjerphan
DOC Rework Importance of Feature Scaling example (scikit-learn#25012)
94fd968 
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-authored-by: Christian Lorentzen <lorentzen.ch@gmail.com>
adrinjalali pushed a commit that referenced this pull request Jan 24, 2023
@ArturoAmorQ @glemaitre
@lorentzenchr @adrinjalali
DOC Rework Importance of Feature Scaling example (#25012)
546a506 
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-authored-by: Christian Lorentzen <lorentzen.ch@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@glemaitre glemaitre glemaitre approved these changes

@lorentzenchr lorentzenchr lorentzenchr approved these changes

Assignees
No one assigned
Labels
Documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add more didactic scaling example?
4 participants
@ArturoAmorQ @glemaitre @lorentzenchr @jjerphan
0