Skip to content

DOC Notebook style and enhanced descriptions in SVM kernels example #26805

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

Merged
merged 29 commits into from
Aug 24, 2023

Conversation

StefanieSenger
Copy link
Contributor

@StefanieSenger StefanieSenger commented Jul 8, 2023

What does this implement/fix? Explain your changes.

This PR suggests to introduce notebook style formatting and enhanced descriptions in the SVM kernels example,

Any other comments?

A few issues:

  1. The math formulas break when also applying a line break. I couldn't find how to fix that.
  2. I'd like to include the sigmoid kernel later, but need to do some reading on it first.

Looking forward to your comments. :)

@github-actions
Copy link

github-actions bot commented Jul 8, 2023

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

Generated for commit: 262eb06. Link to the linter CI: here

Copy link
Member

@adrinjalali adrinjalali left a comment

Choose a reason for hiding this comment

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

I think you can fix the math with this patch:

diff --git a/examples/svm/plot_svm_kernels.py b/examples/svm/plot_svm_kernels.py
index 3c4772fa3..e475d6936 100644
--- a/examples/svm/plot_svm_kernels.py
+++ b/examples/svm/plot_svm_kernels.py
@@ -210,8 +210,8 @@ plot_training_data_with_decision_boundary("linear")
 # The polynomial kernel changes the notion of similarity. The kernel function
 # is defined as:
 #
-# .. math:: K(\mathbf{x}_1, \mathbf{x}_2) = (\gamma \cdot \
-# .. math:: \mathbf{x}_1^\top\mathbf{x}_2 + r)^d
+# .. math::
+#   K(\mathbf{x}_1, \mathbf{x}_2) = (\gamma \cdot \mathbf{x}_1^\top\mathbf{x}_2 + r)^d
 #
 # where :math:`{d}` is the degree (`degree`) of the polynomial, :math:`{\gamma}`
 # (`gamma`) controls the influence of each individual training sample on the
@@ -238,8 +238,9 @@ plot_training_data_with_decision_boundary("poly")
 # similarity between two data points in infinite dimensions and then approaches
 # classification by majority vote. The kernel function is defined as:
 #
-# .. math:: K(\mathbf{x}_1, \mathbf{x}_2) = \exp\left(-\gamma \cdot \
-# .. math:: {\|\mathbf{x}_1 - \mathbf{x}_2\|^2}\right)
+# .. math::
+#   K(\mathbf{x}_1, \mathbf{x}_2) = \exp\left(-\gamma \cdot \
+#       {\|\mathbf{x}_1 - \mathbf{x}_2\|^2}\right)
 #
 # where :math:`{\gamma}` (`gamma`) controls the influence of each individual
 # training sample on the decision boundary.
@@ -258,8 +259,9 @@ plot_training_data_with_decision_boundary("rbf")
 # **************
 # The sigmoid kernel function is defined as:
 #
-# .. math:: K(\mathbf{x}_1, \mathbf{x}_2) = \tanh(\gamma \cdot \\
-# .. math:: \mathbf{x}_1^\top\mathbf{x}_2 + r)
+# .. math::
+#   K(\mathbf{x}_1, \mathbf{x}_2) = \tanh(\gamma \cdot \
+#       \mathbf{x}_1^\top\mathbf{x}_2 + r)
 #
 plot_training_data_with_decision_boundary("sigmoid")

Copy link
Member

@adrinjalali adrinjalali left a comment

Choose a reason for hiding this comment

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

@glemaitre do you wanna have a look?

@adrinjalali
Copy link
Member

@StefanieSenger could you please also add a link to this example in the SVM api docs?

@StefanieSenger
Copy link
Contributor Author

@StefanieSenger could you please also add a link to this example in the SVM api docs?

@adrinjalali Sure. Where exactly do you think of putting it? It is shown automatically at the bottom of sklearn.svm.SVC. Or do you mean the user guide?

@adrinjalali
Copy link
Member

The issue with the ones automatically shown on the bottom of API docs is that it's really not clear what example is relevant for what. Adding the link explicitly in the API doc would make these much more discoverable. I'd add it to the docstring of kernel parameter, or under the Examples section.

@glemaitre
Copy link
Member

@glemaitre do you wanna have a look?

Yep I will have a look now.

@glemaitre glemaitre self-requested a review July 11, 2023 09:42
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.

I propose some simplification regarding the plotting part.
I will check the narrative in early afternoon.

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.

I am fine with the rest of the example.

@StefanieSenger
Copy link
Contributor Author

Thanks for your review @glemaitre. I have substituted the old matplotlib plotting by our DecisionBoundaryDisplay and made some other small adjustments that you suggested. :)

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.

Just a couple of matplotlib simplifications.

Otherwise LGTM.

StefanieSenger and others added 7 commits August 18, 2023 11:16
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
StefanieSenger and others added 4 commits August 18, 2023 22:10
Co-authored-by: Arturo Amor <86408019+ArturoAmorQ@users.noreply.github.com>
Co-authored-by: Arturo Amor <86408019+ArturoAmorQ@users.noreply.github.com>
Co-authored-by: Arturo Amor <86408019+ArturoAmorQ@users.noreply.github.com>
Co-authored-by: Arturo Amor <86408019+ArturoAmorQ@users.noreply.github.com>
@StefanieSenger StefanieSenger marked this pull request as draft August 19, 2023 05:40
@StefanieSenger
Copy link
Contributor Author

Thank you, @ArturoAmorQ for your comments. You are right with your procaution regarding the margins being at a distance from the boundary equal to the C param on the linear kernel. I was assuming something empirically, that might as well have been coincidence. I have deleted that information to be sure.

I have modified the supervised_learning.rst file, that refers to the plots, and added the sigmoid plot there, too.

Unfortunately I have pushed some wrong files along with the changes (was late yesterday). I will take care of this, but I am not sure if I have time for this before Wednesday. I have turned this PR into a draft in the meantime.

@StefanieSenger StefanieSenger marked this pull request as ready for review August 23, 2023 08:54
@StefanieSenger
Copy link
Contributor Author

I've now deleted the accidentally pushed files and I hope I did it in the cleanest possible way. Sorry for adding confusion.

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.

Just some final nitpicks :)

StefanieSenger and others added 5 commits August 23, 2023 20:48
Co-authored-by: Arturo Amor <86408019+ArturoAmorQ@users.noreply.github.com>
Co-authored-by: Arturo Amor <86408019+ArturoAmorQ@users.noreply.github.com>
Co-authored-by: Arturo Amor <86408019+ArturoAmorQ@users.noreply.github.com>
Co-authored-by: Arturo Amor <86408019+ArturoAmorQ@users.noreply.github.com>
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.

LGTM, thanks for the PR @StefanieSenger! 🎉

On a side note, this is going to be my very first merge on scikit-learn :)

@ArturoAmorQ ArturoAmorQ merged commit 4906ce5 into scikit-learn:main Aug 24, 2023
@StefanieSenger
Copy link
Contributor Author

On a side note, this is going to be my very first merge on scikit-learn :)

Yeahhhh, let's celebrate, @ArturoAmorQ 🎉🎉🎉

@jjerphan
Copy link
Member

Thank you, @StefanieSenger!

And congratulations, @ArturoAmorQ! 🎉

With great power comes great responsibility. 🙂

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

Successfully merging this pull request may close these issues.

5 participants