Skip to content
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

DOC Add link to Quantile example in Gradient Boosting #30266

Merged
merged 3 commits into from
Nov 15, 2024

Conversation

Akankshaaaa
Copy link
Contributor

Reference Issues/PRs

References #26927

What does this fix?

Added links to gradient boosting quantile example in docs

Copy link

github-actions bot commented Nov 12, 2024

✔️ Linting Passed

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

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

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.

Thanks for your contribution, @Akankshaaaa.

The references are well placed, but I think the first one at the loss param is enough, since all the use cases when people would set the alpha param involve a certain loss anyways and then users would see the same link in two different places for the same use case, which is too much.

Thus, I would suggest to remove the second reference.

And a little nit on the first reference.

sklearn/ensemble/_gb.py Outdated Show resolved Hide resolved
Co-authored-by: Stefanie Senger <[email protected]>
@Akankshaaaa
Copy link
Contributor Author

Thanks for your contribution, @Akankshaaaa.

The references are well placed, but I think the first one at the loss param is enough, since all the use cases when people would set the alpha param involve a certain loss anyways and then users would see the same link in two different places for the same use case, which is too much.

Thus, I would suggest to remove the second reference.

And a little nit on the first reference.

Yes, that makes sense. I have committed your suggestion.

@StefanieSenger
Copy link
Contributor

Yes, and I would also remove the second reference, @Akankshaaaa.

removed second reference
@Akankshaaaa
Copy link
Contributor Author

@StefanieSenger done.

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.

Nice! Thank you.
I think this can now be merged. 🍾

Would you mind, @adrinjalali ?

Copy link
Contributor

@OmarManzoor OmarManzoor 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 @Akankshaaaa

@OmarManzoor OmarManzoor merged commit 6f12d3f into scikit-learn:main Nov 15, 2024
30 checks passed
jeremiedbb pushed a commit to jeremiedbb/scikit-learn that referenced this pull request Dec 4, 2024
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