Skip to content

DOC Removed examples/semi_supervised/plot_semi_supervised_versus_svm_iris.py#31625

Closed
elhambbi wants to merge 1 commit intoscikit-learn:mainfrom
elhambbi:cont04
Closed

DOC Removed examples/semi_supervised/plot_semi_supervised_versus_svm_iris.py#31625
elhambbi wants to merge 1 commit intoscikit-learn:mainfrom
elhambbi:cont04

Conversation

@elhambbi
Copy link
Copy Markdown
Contributor

Reference Issues/PRs

Removed the example examples/semi_supervised/plot_semi_supervised_versus_svm_iris.py as discussed in #31499

What does this implement/fix? Explain your changes.

The performance of semi-supervised methods, SelfTraining and LabelSpreading, is already evaluated in this example, which is improved in #31104. There is no need for more examples to show this.

Any other comments?

@github-actions
Copy link
Copy Markdown

✔️ Linting Passed

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

Generated for commit: c7542e7. Link to the linter CI: here

@elhambbi
Copy link
Copy Markdown
Contributor Author

Hi @StefanieSenger. I created this PR to remove the example mentioned in #31499 (this example is not referenced anywhere).
I'll also check if we need more improvement for the other example in #31104 because of this removal. Thanks.

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.

Hi @elhambbi, thanks for this PR to remove the example as we had discussed with @adrinjalali.

There are a few tasks that are left to be done:

  1. Remove the references to this example in doc/modules/semi_supervised.rst (in currently li. 66 and in li. 139)
  2. We need to add a redirection in doc/conf.py to the re-worked example in case the removed example had been linked externally. You can use #30906 as a guide of how to do that.
  3. Could you shortly sum up, what are the interesting things from the deleted example, that you would like to integrate into your re-worked example in #31104? Are there any at all?

No worries about the failing CI checks, btw, they come from main and have nothing to do with your PR.

@StefanieSenger
Copy link
Copy Markdown
Member

This PR is a follow up to #31104 which is now merged. The decision to use the plot_semi_supervised_versus_svm_iris.py example to improve the plot_semi_supervised_newsgroups.py example and then to remove it was made here.

@ArturoAmorQ, do you agree? I wasn't sure, since @elhambbi wrote you might have suggested something else (here), but I didn't find where you did it.

@elhambbi, there are a few things left to be done before we can merge this PR. Please have a look at the link in the comment above as an example of how to remove an example and add a redirection.

@StefanieSenger
Copy link
Copy Markdown
Member

Oh, now I have found #32024, which improves the plot_semi_supervised_versus_svm_iris.py example. Sorry, I haven't seen it before, @ArturoAmorQ.

Then let's close this PR?

@ArturoAmorQ
Copy link
Copy Markdown
Member

If you both agree that #32024 is worth keeping, then yes, we would have to close this PR.
Sorry for not having linked it here for better visibility !
But I'm happy to see your enthusiasm and cotributions @elhambbi :)

@StefanieSenger
Copy link
Copy Markdown
Member

Yes, I think #32024 is really worth keeping, as it now nicely explains predict_proba and gives this example a new focus (and a new reason to be kept). I will come to review it further in the following weeks. Meanwhile, maybe @elhambbi you are interested in trying to review it?

@StefanieSenger
Copy link
Copy Markdown
Member

We've discussed that we want to keep the example and @ArturoAmorQ has opened a PR #32024 to improve it.

Therefore, I am closing this PR.

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