Skip to content

DOC added link to example plot_svm_margin.py #30975

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

Closed
wants to merge 0 commits into from

Conversation

SwathiR1999
Copy link

@SwathiR1999 SwathiR1999 commented Mar 11, 2025

Reference Issues/PRs

References #30621

What does this implement/fix? Explain your changes.

This adds a reference to a visual example demonstrating the impact of the C parameter in SVM classification, making it easier for users to understand its effect on the decision boundary.

Copy link

github-actions bot commented Mar 11, 2025

✔️ Linting Passed

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

Generated for commit: 54f6046. Link to the linter CI: here

@SwathiR1999 SwathiR1999 marked this pull request as draft March 11, 2025 14:11
@SwathiR1999 SwathiR1999 marked this pull request as ready for review March 11, 2025 14:12
Copy link
Contributor

@StefanieSenger StefanieSenger left a comment

Choose a reason for hiding this comment

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

Thanks for working on this, @SwathiR1999.

However, I see two problems here:

  1. Placing this example in the "1.4.5. Tips on Practical Use" section is not a good spot for this.
  2. The example doesn't seem to show what it claims: The effect on the C param on the margin is not demonstrated.

As I would all in all consider this example to not add much value, I would suggest to re-purpose this PR to remove the example instead. Would you agree to that, @adrinjalali? Would you be willing to work on that, @SwathiR1999?

@SwathiR1999
Copy link
Author

Thanks for the feedback, @StefanieSenger ! I understand the concerns. Would you prefer that I remove the example, or should I try improving it to better demonstrate the effect of C on the margin?

@StefanieSenger
Copy link
Contributor

StefanieSenger commented Mar 15, 2025

Hi @SwathiR1999,

good to know that you're ready for both options.

Thanks for the feedback, @StefanieSenger ! I understand the concerns. Would you prefer that I remove the example, or should I try improving it to better demonstrate the effect of C on the margin?

Let's wait for @adrinjalali's opinion on that. I cannot decide that.

@adrinjalali
Copy link
Member

This is a very important message when it comes to SVMs actually. But I agree this example is far from ideal. Looking at the examples, I see plot_separating_hyperplane.py could also see some improvements. I suggest merging the two examples and adding good explanations about the messages they provide.

@SwathiR1999
Copy link
Author

Hi @adrinjalali , @StefanieSenger ,

Thanks for the feedback! I’ll work on merging plot_svm_margin.py with plot_separating_hyperplane.py and improving the explanations. Would you prefer that I update this PR with the changes, or should I open a new PR for the merged example?

Let me know how you'd like to proceed.

Thanks!

@adrinjalali
Copy link
Member

Doing it in this PR is fine, we can always rename the PR anyway.

@StefanieSenger StefanieSenger requested review from StefanieSenger and removed request for StefanieSenger March 20, 2025 17:14
@StefanieSenger
Copy link
Contributor

Hi @SwathiR1999, please ping me when you're finished for the moment and want a review.

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.

3 participants