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

Update Readme and docstring for perceptron #10559

Merged

qinhanmin2014 merged 2 commits into scikit-learn:master from syashakash:edit-doc-perceptron

Apr 12, 2018

Contributor

syashakash commented Jan 31, 2018

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Fixes #10544

Any other comments?

Haven't checked by generating doc locally due to internal issues. Please review and recommend change. The edited description is direct but could be better.

syashakash changed the title ~~[WIP] Update Readme and docstring for perceptron~~ Update Readme and docstring for perceptron

rsivapr reviewed

View reviewed changes

Contributor

rsivapr left a comment

Note that BaseSGDClassifier will combine binary classifiers under-the-hood when there are greater than 2 classes.

Refer:

scikit-learn/sklearn/linear_model/stochastic_gradient.py

Line 396 in a24c8b4

if n_classes > 2:

doc/modules/linear_model.rst Outdated

@@ @@ -838,7 +838,7 @@ Perceptron @@
               ==========
               The :class:`Perceptron` is another simple algorithm suitable for large scale
-              learning. By default:
+              learning. It is a binary classifier. By default:

Contributor

rsivapr Feb 1, 2018

I would just say:

The Perceptron is another simple classification algorithm suitable for large scale learning. By default:

Contributor Author

syashakash Feb 1, 2018

Thanks for the suggestion. I have done the necessary changes

sklearn/linear_model/perceptron.py Outdated

@@ @@ -96,6 +96,7 @@ class Perceptron(BaseSGDClassifier): @@
                   Notes
                   -----
+                  `Perceptron` is a binary classifier.
                   `Perceptron` and `SGDClassifier` share the same underlying implementation.

Contributor

rsivapr Feb 1, 2018

And here:

Perceptron and SGDClassifier are both classifiers that share the same underlying implementation.

jnothman reviewed

View reviewed changes

Member

jnothman left a comment

Please keep line lengths under 80 characters. Thanks @rsivapr for your review

syashakash force-pushed the edit-doc-perceptron branch from 0f4f91b to 298f869 Compare

February 2, 2018 05:23

Member

glemaitre commented Feb 8, 2018

@syashakash Please fix PEP8

syashakash force-pushed the edit-doc-perceptron branch 3 times, most recently from 2057933 to 37c8de1 Compare

February 14, 2018 19:54

Contributor Author

syashakash commented Feb 18, 2018

@glemaitre please review

qinhanmin2014 reviewed

View reviewed changes

Member

qinhanmin2014 left a comment

Please avoid unrelevant changes.

mohamed-ali suggested changes

View reviewed changes

doc/modules/linear_model.rst Outdated

@@ @@ -14,7 +14,7 @@ value. @@
               .. math::    \hat{y}(w, x) = w_0 + w_1 x_1 + ... + w_p x_p
               Across the module, we designate the vector :math:`w = (w_1,
-              ..., w_p)` as ``coef_`` and :math:`w_0` as ``intercept_``.
+                      ..., w_p)` as ``coef_`` and :math:`w_0` as ``intercept_``.

Contributor

mohamed-ali Feb 26, 2018

This is an irrelevant change. Please avoid it.

doc/modules/linear_model.rst Outdated

@@ @@ -34,9 +34,9 @@ solves a problem of the form: @@
               .. math:: \underset{w}{min\,} {|| X w - y||_2}^2
               .. figure:: ../auto_examples/linear_model/images/sphx_glr_plot_ols_001.png
-                 :target: ../auto_examples/linear_model/plot_ols.html
+                  :target: ../auto_examples/linear_model/plot_ols.html

Contributor

mohamed-ali Feb 26, 2018

This is an irrelevant change. Please avoid it.

doc/modules/linear_model.rst Outdated

                  :align: center
-                 :scale: 50%
+                         :scale: 50%

Contributor

mohamed-ali Feb 26, 2018

This is an irrelevant change. Please avoid it.

doc/modules/linear_model.rst Outdated

               of shrinkage and thus the coefficients become more robust to collinearity.
               .. figure:: ../auto_examples/linear_model/images/sphx_glr_plot_ridge_path_001.png
-                 :target: ../auto_examples/linear_model/plot_ridge_path.html
+                  :target: ../auto_examples/linear_model/plot_ridge_path.html

Contributor

mohamed-ali Feb 26, 2018

This is an irrelevant change. Please avoid it.

doc/modules/linear_model.rst Outdated

                  :align: center
-                 :scale: 50%
+                         :scale: 50%

Contributor

mohamed-ali Feb 26, 2018

This is an irrelevant change. Please avoid it.

doc/modules/linear_model.rst Outdated

                  :align: center
-                 :scale: 50%
+                         :scale: 50%

Contributor

mohamed-ali Feb 26, 2018

avoid this

doc/modules/linear_model.rst Outdated


		The loss function that :class:`HuberRegressor` minimizes is given by
		The loss function that :class:`HuberRegressor` minimizes is given by

Contributor

mohamed-ali Feb 26, 2018

avoid this as well.

doc/modules/linear_model.rst Outdated


		This figure is created using the :class:`PolynomialFeatures` preprocessor.
		This figure is created using the :class:`PolynomialFeatures` preprocessor.

Contributor

mohamed-ali Feb 26, 2018

avoid this

doc/modules/linear_model.rst Outdated

               polynomial features of varying degrees:
               .. figure:: ../auto_examples/linear_model/images/sphx_glr_plot_polynomial_interpolation_001.png
-                 :target: ../auto_examples/linear_model/plot_polynomial_interpolation.html
+                  :target: ../auto_examples/linear_model/plot_polynomial_interpolation.html

Contributor

mohamed-ali Feb 26, 2018

avoid this change as well

doc/modules/linear_model.rst Outdated

                  :align: center
-                 :scale: 50%
+                         :scale: 50%

Contributor

mohamed-ali Feb 26, 2018

avoid this change.

Contributor

mohamed-ali commented Feb 26, 2018

@syashakash, I see that this pull request is pending, waiting for some edits from your behalf. I reviewed the code to help you see the changes that you need to undo, since they are irrelevant for this PR.

@qinhanmin2014, can you confirm that I didn't forget any other change that need to be done?

Member

qinhanmin2014 commented Feb 27, 2018

Thanks @mohamed-ali for the review. I'm not sure what's happening here but it seems that the best way is to open another PR instead of modifying this one. @mohamed-ali you can take it if @syashakash still don't reply after a couple of days.

Contributor

mohamed-ali commented Feb 27, 2018

@qinhanmin2014 yes sure, just after I finish working on this #10711. It would be helpful if you review the work on progress and give me your recommendations :)

syashakash force-pushed the edit-doc-perceptron branch 2 times, most recently from 8ed4ca1 to 6be37b1 Compare

March 4, 2018 11:11

Contributor Author

syashakash commented Mar 5, 2018

@qinhanmin2014 @mohamed-ali I hope the changes you have suggested are done properly. The mess was created because I used autopep8. I have tried to rectify manually so please if you could review so that this PR is done with.
PS. Sorry for late reply.

Member

qinhanmin2014 commented Mar 5, 2018

@syashakash There's still many unrelevant changes. Please revert all of them or simply open another PR.

Contributor

mohamed-ali commented Mar 5, 2018

@syashakash, I don't know if you have already done that, but you can look at the file differences here: https://github.com/scikit-learn/scikit-learn/pull/10559/files just undo anything green from the right side that is not subject to this issue.

syashakash force-pushed the edit-doc-perceptron branch from 6be37b1 to 6080581 Compare

March 6, 2018 13:53

qinhanmin2014 reviewed

View reviewed changes

doc/modules/linear_model.rst Outdated

    
              The :class:`Perceptron` is another simple algorithm suitable for large scale

              learning. By default:

              The :class:`Perceptron` is another simple classification algorithm suitable for

              large scale learning. It is a binary classifier. By default:

Member

qinhanmin2014 Mar 8, 2018

It's not a binary classifier?

Contributor Author

syashakash Mar 22, 2018

I didn't get you. It says perceptron is a binary classifier.

Member

qinhanmin2014 Mar 22, 2018

You say perceptron is a binary classifier, but it's not a binary classifier.

Member

qinhanmin2014 Mar 22, 2018

It supports multi-class classification


          Update Readme and docstring for perceptron

a594359

syashakash force-pushed the edit-doc-perceptron branch from 6080581 to a594359 Compare

March 22, 2018 16:36


          improvement

b1f5346

qinhanmin2014 approved these changes

View reviewed changes

Member

qinhanmin2014 left a comment

LGTM, will merge when green.

qinhanmin2014 merged commit 07a0b18 into scikit-learn:master

syashakash deleted the edit-doc-perceptron branch

April 12, 2018 11:55

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