Add _voting.py file with voting regressor implementation #29724
Conversation
|
Hey @adrinjalali , could you please check this PR and merge this ? |
|
@lesteve Could you please check and merge this PR |
|
Hey @lesteve , could you please tell me what is the error in the PR ? |
|
The CI issue is being investigated and hopefully fixed soon in #29771, so it's not related to your PR. Seems like you put the link to the example at the end of the docstring but I am wondering whether the intent of #26927 is to put the example link before the parameters for example after the "For more details read the User Guide". Otherwise the link to the example is in the middle of a very verbose page, and IMO this does not really help discoverability ... any thoughs on this @adrinjalali? You can view the rendering of your PR by clicking on "check the rendered doc link" and then to the class you modified the docstring ( |
|
@doshi-kevin also I am personally not a big fan of being pinged unless this is really blocking and I happen to be the best person to fix something quickly. Different people have different opinions on this. I would say waiting roughly a week without answer before pinging is probably a reasonable guideline, see for example this FAQ section |
|
I am really sorry, I will make the changes. Sorry for any inconveniences caused. |
|
@doshi-kevin the issue in #29771 is fixed now. Can you pull the changes from upstream main into your branch? |
|
Hey @marenwestermann , I checked other similar PRs and I believe I have added the link to the correct location in the file. I added a little text of 'please refer to,'. Please check this PR, in case of any changes, I will be happy to do it. |
|
@marenwestermann Any updates on this ? |
|
I agree with @marenwestermann's comment here: https://github.com/scikit-learn/scikit-learn/pull/29724/files#r1743929175 @lesteve it depends on whether the example is focusing on the estimator as a whole, or a specific parameter. I believe in this case having it at the end of the docstring is the right place. |
|
yes, I will make the text shorter. |
|
The only thing you need to add is:
|
|
Hey @adrinjalali , made the changes, sorry for taking too much of your time |
Co-authored-by: Yao Xiao <108576690+Charlie-XIAO@users.noreply.github.com>
|
Thank you so much @Charlie-XIAO , I have made the changes. |
There was a problem hiding this comment.
Thanks @doshi-kevin, I'm personally okay with this now, the rendered docs is here: https://output.circle-artifacts.com/output/job/6fbaa7c6-84ad-49bb-8383-9c98a2486271/artifacts/0/doc/modules/generated/sklearn.ensemble.VotingRegressor.html
But indeed as mentioned in #29724 (comment) this could have been moved to above parameters, i.e., right after or right before the "read more in user guide" line. I did not follow this meta-issue but it seems there are related PRs doing in both ways. For this specific case it's probably better to move it upwards (I wonder if one can find where the link is from the screenshot below at first glance 🤔)
|
@Charlie-XIAO Noted. I will move the link upwards (above "read more in user guide" line) |
|
@Charlie-XIAO @adrinjalali I have made the change and put the link in the beginning of the page above ...see more User Guide. Please check once. Thank You. |
Co-authored-by: Yao Xiao <108576690+Charlie-XIAO@users.noreply.github.com>
|
Everything looks good now, thanks @doshi-kevin, I'm enabling auto-merge. |
Reference Issues/PRs
#26927
What does this implement/fix? Explain your changes.
Includes a link to the plot_voting_regressor.py example.
Any other comments?
Please review the code once and correct is any mistakes. I am a beginner and would love to be a contributor of such a large open-source project like scikit-learn.