Skip to content

DOC remove obsolete SVM example #27108

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 5 commits into from
May 19, 2024
Merged

Conversation

adrinjalali
Copy link
Member

@adrinjalali adrinjalali commented Aug 18, 2023

Closes #26972

xref: #26972 (comment) and #27151

This example is pretty useless and outdated. Can simply be removed.

cc @glemaitre

@github-actions
Copy link

github-actions bot commented Aug 18, 2023

✔️ Linting Passed

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

Generated for commit: 3e5c08e. Link to the linter CI: here

@@ -299,6 +299,7 @@
"auto_examples/decomposition/plot_beta_divergence": (
"auto_examples/applications/plot_topics_extraction_with_nmf_lda"
),
"auto_examples/svm/plot_svm_nonlinear": "auto_examples/svm/plot_svm_kernels",
Copy link
Member

Choose a reason for hiding this comment

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

As noted in #26972 (comment), the XOR problem is the canonical example for non-linear decision functions.

I agree that auto_examples/svm/plot_svm_kernels illustrates similar motivations but I would rather redirect to an example that also includes the canonical XOR, maybe with more models / kernels as suggested by Guillaume.

Copy link
Member Author

Choose a reason for hiding this comment

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

@ogrisel Do you think these days this example is still relevant? I would rather people read more realistic examples instead. Is there another example you prefer to be here instead?

I also don't really see a way where users would actually benefit from this redirection. Our example pages don't have a link to the same example in the new release. The only link we provide is to the home page of the latest release, not the same page in the latest release. So I'd be happy to remove these redirections altogether.

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking about adding XOR to classifier comparison, wdyt?

Copy link
Member

Choose a reason for hiding this comment

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

I would be inclined to have this dataset because this is still an interesting synthetic one. Probably the classification one is the right place to have it. For instance, this is part of the default dataset in this playground (https://playground.tensorflow.org)

Copy link
Member

Choose a reason for hiding this comment

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

So let's remove this one, and put the dataset into an existing one. We really have too many examples!

@TamaraAtanasoska
Copy link
Contributor

Just commenting to say that I will start looking into implementing the suggestions from the comments of this issue into a new, updated example.

@adrinjalali adrinjalali requested a review from ogrisel October 17, 2023 12:08
@marenwestermann
Copy link
Member

If plot_svm_nonlinear.py gets removed, there is no example anymore for demonstrating how to use svm.NuSVC. Should I open an issue for writing an example for this estimator?

@lorentzenchr
Copy link
Member

@marenwestermann Could NuSVC added in another existing example? The general idea is to have less examples.
I could also live with just remaining this one ( => I approved).

@marenwestermann
Copy link
Member

@lorentzenchr yes, adding an example of NuSVC to an existing example is also a good idea in my opinion. @ogrisel What's your opinion on this?

@lorentzenchr lorentzenchr added this to the 1.5 milestone Apr 10, 2024
@adrinjalali
Copy link
Member Author

This now adds the XOR pattern to the plot_svm_kernels example.

@glemaitre glemaitre self-requested a review May 18, 2024 10:27
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.

LGTM. Thanks to have keep the XOR in one example. I think that it fits well in the kernel example.

@glemaitre glemaitre merged commit 18c1972 into scikit-learn:main May 19, 2024
30 checks passed
jeremiedbb pushed a commit to jeremiedbb/scikit-learn that referenced this pull request May 20, 2024
@jeremiedbb jeremiedbb mentioned this pull request May 20, 2024
14 tasks
jeremiedbb pushed a commit that referenced this pull request May 21, 2024
@adrinjalali adrinjalali deleted the svm/remove branch May 23, 2024 18:00
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.

7 participants