Skip to content

DOC added link to example plot_svm_margin.py#30975

Closed
SwathiR1999 wants to merge 0 commit intoscikit-learn:mainfrom
SwathiR1999:doc_addlink
Closed

DOC added link to example plot_svm_margin.py#30975
SwathiR1999 wants to merge 0 commit intoscikit-learn:mainfrom
SwathiR1999:doc_addlink

Conversation

@SwathiR1999
Copy link
Copy Markdown

@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.

@github-actions
Copy link
Copy Markdown

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
Copy Markdown
Member

@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
Copy Markdown
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
Copy Markdown
Member

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

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