Skip to content

DOC add link to plot_voting_probas.py for Voting Classifier in ensemble.rst #30847

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 11 commits into from
Mar 18, 2025

Conversation

elhambbi
Copy link
Contributor

Reference Issues/PRs

Towards #30621

What does this implement/fix? Explain your changes.

The examples for Voting Classifier were missed.
I have added two examples plot_voting_decision_regions.py and plot_voting_probas.py for Voting Classifier in ensemble.rst

Any other comments?

Copy link

github-actions bot commented Feb 17, 2025

✔️ Linting Passed

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

Generated for commit: 36d22fb. Link to the linter CI: here

Comment on lines 1650 to 1653
.. rubric:: Examples

* :ref:`sphx_glr_auto_examples_ensemble_plot_stack_predictors.py`

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @elhambbi,

thank you, but it seems something went wrong here. You had already added this example in #30747, which was merged into main 6 days ago.

Please, before moving forward, update your main branch with

git fetch upstream
git rebase upstream/main

to pull the latest changes and then either merge these changes into your feature branch or (easier) create a new branch.

Please do then re-evaluate this PR and push again or (easier) make a new PR from your new branch.

Thank you for your efforts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

HIi @StefanieSenger . Thanks for your feedback.
I merged the changes from the main into my branch and pushed again.
It should be ok now.
Thank you.

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.

Thank you a lot for your further work, @elhambbi!

I would have two suggestions that would lead to one reference not being added and the other one added but in another spot. I hope that is okay for you.

The link to plot_voting_probas.py is a valuable addition, since it had not been linked anywhere before. Though I would think it should go in the section "1.11.4.3. Weighted Average Probabilities (Soft Voting)" where soft voting is treated, as in the example. And it would be nice if you can integrate this into the text, not in an examples section.

Be aware that if you add a link like this :ref:title ``, you can change its title so that not the example's title is shown but whatever you pick. This way it can integrate more nicely into the sentences.

Do you think that would fit with you?

@@ -1485,6 +1485,11 @@ Optionally, weights can be provided for the individual classifiers::
>>> grid = GridSearchCV(estimator=eclf, param_grid=params, cv=5)
>>> grid = grid.fit(iris.data, iris.target)

.. rubric:: Examples

* :ref:`sphx_glr_auto_examples_ensemble_plot_voting_decision_regions.py`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* :ref:`sphx_glr_auto_examples_ensemble_plot_voting_decision_regions.py`

I would rather leave this one out, since it is linked in line 1445 with an image and that is pretty close.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you @StefanieSenger .
I removed reference to plot_voting_decision_regions.py and added plot_voting_probas.py within the related text :). Let me know if you prefer to modify it.

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.

Thank you for your work, @elhambbi!

I have two nits, but these are both details. Feel free to ignore.

Do you think we can merge this one, @adrinjalali or @marenwestermann?

@@ -1485,6 +1486,7 @@ Optionally, weights can be provided for the individual classifiers::
>>> grid = GridSearchCV(estimator=eclf, param_grid=params, cv=5)
>>> grid = grid.fit(iris.data, iris.target)


Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

Comment on lines 1414 to 1415
highest average probability. See :ref:`plot probabilities <sphx_glr_auto_examples_ensemble_plot_voting_probas.py>` for
a detailed illustration of class probabilities averaged by soft voting.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
highest average probability. See :ref:`plot probabilities <sphx_glr_auto_examples_ensemble_plot_voting_probas.py>` for
a detailed illustration of class probabilities averaged by soft voting.
highest average probability. See this example on
:ref:`Visualising class probabilities in a Voting Classifier <sphx_glr_auto_examples_ensemble_plot_voting_probas.py>`
for a detailed illustration of class probabilities averaged by soft voting.

In the hope this more respects our max character length of 88 per line.

And removing "plot" from the title, which is more like an intern naming convention. Does that resonate with you? Feel free to change again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you @StefanieSenger.
I made the changes to maintain consistency with ScikitLearn standards.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for modifying this @elhambbi!

@StefanieSenger
Copy link
Contributor

Hallo @elhambbi,

I am sorry, but it seems that you have pushed the reference to this new example to the same branch. I've seen this happening if people use tools like VSCode to manage git, when they don't set that each new local branch should create a new remote branch and then accidentally they push to the wrong branch.

If this is what happened, then I would recommend to learn some more git. (I really liked the Oh my git! game for learning git, but since you need to download it, let me add: no garantee that it's alright, I didn't check.)

Let's link to different examples in separate branches. The reason for it is that it is easier for maintainers to review and merge. Can you please remove this new link and push again?

@elhambbi
Copy link
Contributor Author

Hi @StefanieSenger sorry for the conflict.
I removed the new example here and pushed it in another branch and created a new PR : DOC add link to plot_semi_supervised_newsgroups.py example in semi_supervised.rst #30882.
Now, the two PRs are distinct and can be evaluated separately.
Thank you.

@StefanieSenger
Copy link
Contributor

StefanieSenger commented Feb 23, 2025

Thank you @elhambbi!

This now can be reviewed by a maintainer. @marenwestermann @adrinjalali, do you want to have a look?

@adrinjalali adrinjalali changed the title DOC add examples plot_voting_decision_regions.py and plot_voting_probas.py for Voting Classifier in ensemble.rst DOC add link to plot_voting_probas.py for Voting Classifier in ensemble.rst Mar 18, 2025
@adrinjalali adrinjalali enabled auto-merge (squash) March 18, 2025 16:53
@adrinjalali adrinjalali merged commit efc355e into scikit-learn:main Mar 18, 2025
31 checks passed
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