-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
DOC Merge plot_svm_margin.py and plot_separating_hyperplane.py into plot_svm_hyperplane_margin.py #31045
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
base: main
Are you sure you want to change the base?
Conversation
❌ Linting issuesThis PR is introducing linting issues. Here's a summary of the issues. Note that you can avoid having linting issues by enabling You can see the details of the linting issues under the
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @SwathiR1999,
thank you for your patience and for your efforts to merge the examples. It looks very good and I like the design of the plots. The plots all have heading and legend and are easy on the eye.
I think we could add some information and I have left you some suggestions on that, too.
When I build the docs locally, I cannot click to open it and also there is some text overflow in the background. I believe this is because of a rending issue in the heading, and I have added a suggestions for a fix.
What is lacking is to remove the old examples and to add redirections from both old links to the new example link in the doc/conf.py
file. You can have a look at #30028 to see how to add the redirections.
You need to delete all links to the old examples as well (and replace it with the new one if suitable). There are some, probably too many, links to plot_separating_hyperplane_unbalanced
currently.
- **Small C (e.g., 0.05)**: | ||
- Allows some misclassifications, resulting in a wider margin. | ||
- **Moderate C (e.g., 1)**: | ||
- Balances classification accuracy and margin width. | ||
- **Large C (e.g., 1000)**: | ||
- Prioritizes classifying all points correctly, leading to a narrower margin. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think while reworking the example, we could add some more information.
Do you think you could add some suggestion of when to use a small, a moderate or a high C value in a practical use case?
We could also talk about overfitting and underfitting in this context.
Also, we could talk about miss-classification / error for different margins.
I think this could be added after the code for the plot using the # %
syntax to make this example more notebook-like. (Also have a look at #30028 for a good example on the notebook style.)
""" | ||
========================================================================= | ||
SVM: Effect of Regularization (C) on Maximum Margin Separating Hyperplane | ||
======================================================================== |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
======================================================================== | |
========================================================================= |
The lacking "=" leads to the title not being rendered correctly. Adding "=" should fix it.
I also hope that this fixes the problem with the example not being clickable from the main examples page and the text overflow there (see other comment).
clf.fit(X, y) | ||
|
||
plt.subplot(1, 3, i) | ||
plt.scatter(X[:, 0], X[:, 1], c=y, s=30, cmap=plt.cm.Paired, edgecolors="k") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add axis labels. Something simple like "Feature 1" and "Feature 2" would be enough.
s=100, | ||
linewidth=1.5, | ||
facecolors="none", | ||
edgecolors="r", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about having a different color for miss-classified samples?
…nto doc_addlink
Hi @StefanieSenger , thanks for the feedback!
Just to clarify - should I also apply similar changes (merge/update/remove references) to |
Oh, my bad. I didn't see there were |
Reference Issues/PRs
References #30621
What does this implement/fix? Explain your changes.
plot_svm_margin.py
andplot_separating_hyperplane.py
) into a single fileplot_svm_hyperplane_margin.py
.Any other comments?
This PR addresses the feedback from #30975, which was closed.
@StefanieSenger Looking forward to your feedback. Thanks!