[MRG+1] Misleading gamma description (#8699) #8716

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

agramfort merged 7 commits into scikit-learn:master from mppaskov:is8699

Apr 22, 2017

Contributor

mppaskov commented Apr 7, 2017 •

edited

Loading

Reference Issue

Fixes #8699

What does this implement/fix? Explain your changes.

Correct description of the gamma parameter and where it is used.

Any other comments?

This is my first PR so hopefully I did it correctly.

agramfort reviewed

View reviewed changes

sklearn/decomposition/kernel_pca.py Outdated

    
                  gamma : float, default=1/n_features

                      Kernel coefficient for rbf and poly kernels. Ignored by other

                      kernels.

                      Kernel coefficient for rbf, poly kernels, sigmoid, laplacian and chi2.

Member

agramfort Apr 8, 2017

Kernel coefficient for rbf, poly, sigmoid, laplacian and chi2 kernels.

I think this might need to be fixed in many places. Do a git grep "gamma :"

Contributor Author

mppaskov commented Apr 8, 2017

@agramfort You are correct I missed them. Thank you for the tip with git grep. The new commit should have addressed them all. The only occurrences of 'gamma' that have not been changed are in the documentation for each individual kernel, and in 'Orthogonal Matching Pursuit' which uses parameter gamma but that is something else.

TomDLT suggested changes

View reviewed changes

Member

TomDLT left a comment

Correct me if I am wrong, but it seems you have not check if all kernels are valid in each class.
A lot of your doc changes are not valid, please double check my comments and update the doc accordingly.

sklearn/manifold/spectral_embedding_.py Outdated

    
                  gamma : float, optional, default : 1/n_features

                      Kernel coefficient for rbf kernel.

                      Kernel coefficient for rbf, poly, sigmoid, laplacian and chi2 kernels.

Member

TomDLT Apr 14, 2017

I don't think it is correct here. The other kernel are not used here.

sklearn/semi_supervised/label_propagation.py Outdated

    
                  gamma : float

                      Parameter for rbf kernel

                      Kernel coefficient for rbf, poly, sigmoid, laplacian and chi2 kernels.

Member

TomDLT Apr 14, 2017

not correct either. Only 'knn', 'rbf' strings are valid inputs

sklearn/semi_supervised/label_propagation.py Outdated

    
                  gamma : float

                      Parameter for rbf kernel

                      Kernel coefficient for rbf, poly, sigmoid, laplacian and chi2 kernels.

Member

TomDLT Apr 14, 2017

not correct

sklearn/semi_supervised/label_propagation.py Outdated

    
                  gamma : float

                    parameter for rbf kernel

                      Kernel coefficient for rbf, poly, sigmoid, laplacian and chi2 kernels.

Member

TomDLT Apr 14, 2017

not correct

sklearn/svm/classes.py Outdated

    
                  gamma : float, optional (default='auto')

                      Kernel coefficient for 'rbf', 'poly' and 'sigmoid'.

                      Kernel coefficient for rbf, poly, sigmoid, laplacian and chi2 kernels.

Member

TomDLT Apr 14, 2017

not correct, laplacian and chi2 kernels are not valid

sklearn/svm/classes.py Outdated

    
                  gamma : float, optional (default='auto')

                      Kernel coefficient for 'rbf', 'poly' and 'sigmoid'.

                      Kernel coefficient for rbf, poly, sigmoid, laplacian and chi2 kernels.

Member

TomDLT Apr 14, 2017

not correct, laplacian and chi2 kernels are not valid

sklearn/svm/classes.py Outdated

    
                  gamma : float, optional (default='auto')

                      Kernel coefficient for 'rbf', 'poly' and 'sigmoid'.

                      Kernel coefficient for rbf, poly, sigmoid, laplacian and chi2 kernels.

Member

TomDLT Apr 14, 2017

not correct, laplacian and chi2 kernels are not valid

sklearn/svm/classes.py Outdated

    
                  gamma : float, optional (default='auto')

                      Kernel coefficient for 'rbf', 'poly' and 'sigmoid'.

                      Kernel coefficient for rbf, poly, sigmoid, laplacian and chi2 kernels.

Member

TomDLT Apr 14, 2017

not correct, laplacian and chi2 kernels are not valid

sklearn/svm/classes.py Outdated

    
                  gamma : float, optional (default='auto')

                      Kernel coefficient for 'rbf', 'poly' and 'sigmoid'.

                      Kernel coefficient for rbf, poly, sigmoid, laplacian and chi2 kernels.

Member

TomDLT Apr 14, 2017

not correct, laplacian and chi2 kernels are not valid

mppaskov added 7 commits

April 18, 2017 22:00


          [WIP] KernelPCA gamma description (scikit-learn#8699)

cc3bfd3

* updated description of gamma to reflect correct usage


          Gamma descriptions across other files

c23fcb9

* Updated descriptions to match across files.


          Fixed a flake8 error from travis

cf82088

* locally the ./build_tools/travis/flake8_diff.sh passes


          Reverted changes to classes.py

ff5259a


          Reverted changes to label_propagation.py

aa3fe6d


          Reverted changes to spectral_embedding_py

d4b9d9b


          Fixed changes to kernel_pca.py

dacf3f3

mppaskov force-pushed the is8699 branch from 792c127 to dacf3f3 Compare

April 19, 2017 02:02

Contributor Author

mppaskov commented Apr 19, 2017

@TomDLT Thank you very much for the review and sorry for sloppy job.
I think I have now made the necessary corrections. I was more careful this time, following which kernels can be called from the given function.

TomDLT approved these changes

View reviewed changes

Member

TomDLT left a comment

LGTM

TomDLT changed the title ~~[MRG] Misleading gamma description (#8699)~~ [MRG+1] Misleading gamma description (#8699)

agramfort merged commit 3a3637c into scikit-learn:master

Member

agramfort commented Apr 22, 2017

Sundrique pushed a commit to Sundrique/scikit-learn that referenced this pull request


          [MRG+1] Misleading gamma description (scikit-learn#8699) (scikit-lear…

295eb13

…n#8716)

[DOC] update description of gamma for kernel methods

dmohns pushed a commit to dmohns/scikit-learn that referenced this pull request


          [MRG+1] Misleading gamma description (scikit-learn#8699) (scikit-lear…

30e3664

…n#8716)

[DOC] update description of gamma for kernel methods

dmohns pushed a commit to dmohns/scikit-learn that referenced this pull request


          [MRG+1] Misleading gamma description (scikit-learn#8699) (scikit-lear…

f937a55

…n#8716)

[DOC] update description of gamma for kernel methods

NelleV pushed a commit to NelleV/scikit-learn that referenced this pull request


          [MRG+1] Misleading gamma description (scikit-learn#8699) (scikit-lear…

…n#8716)

[DOC] update description of gamma for kernel methods

paulha pushed a commit to paulha/scikit-learn that referenced this pull request


          [MRG+1] Misleading gamma description (scikit-learn#8699) (scikit-lear…

dd90b86

…n#8716)

[DOC] update description of gamma for kernel methods

maskani-moh pushed a commit to maskani-moh/scikit-learn that referenced this pull request


          [MRG+1] Misleading gamma description (scikit-learn#8699) (scikit-lear…

c937d47

…n#8716)

[DOC] update description of gamma for kernel methods

jwjohnson314 pushed a commit to jwjohnson314/scikit-learn that referenced this pull request


          [MRG+1] Misleading gamma description (scikit-learn#8699) (scikit-lear…

9b06726

…n#8716)

[DOC] update description of gamma for kernel methods

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet