Skip to content

DOC Add link to plot_monotonic_constraints.py in ensemble examples #31471

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

Conversation

shivamchhuneja
Copy link
Contributor

@shivamchhuneja shivamchhuneja commented Jun 2, 2025

Reference Issues/PRs
Towards #30621

What does this implement/fix? Explain your changes.
-Added plot_monotonic_constraints.py example link to ensemble methods, under histogram based gradient boost

Screenshot of local check:

Screenshot 2025-06-02 at 6 42 42 PM

The example showed how to apply monotonic constraints in gradient boosting, which is useful when domain knowledge points that a feature should have a consistent directional effect on the target variable.

For example: in a marketing model, if we know that increasing the number of ad clicks should not decrease the likelihood of a user converting, we can enforce a monotonic constraint to ensure the model respects this expected behavior.
(slightly bland example but I guess this would at least give an idea on why I picked this one up)

Copy link

github-actions bot commented Jun 2, 2025

✔️ Linting Passed

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

Generated for commit: f89fe34. 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.

Hi @shivamchhuneja!

Thanks for making this PR! That looks quiet good. I would just suggest to move the reference into the section "Monotonic Constraints" that is explicitly treating constraints for histogram based boosting, starting from (currently) line 327 in ensemble.rst. There's also already a link there to it in the Examples section, that you could then remove (your reference is better conveying the information of what to expect from the example).

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, @shivamchhuneja.

Now we only need to remove the other reference above, since the link fits better below.

On your other question:
Yes, the approach of contextualizing the examples would be very useful for users to know what to expect before clicking a link. It always depends a bit, because sometimes, the title of the example already says it all.

Also, if you are interested, copy pasting from the issue:

Comment on lines 376 to 377
gradient boosting, including how they can improve generalization when "domain knowledge
is available", see
Copy link
Contributor

@StefanieSenger StefanieSenger Jun 3, 2025

Choose a reason for hiding this comment

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

I forgot to mention that in my previous review, but I was wondering about the quotation marks and I think we can leave them out, since we don't want to communicate anything special with it here.

It's a bit nit picky, but we really try to keep the docs as clean as possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, will make this change and note it for the future

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All the changes are done with the current PR.

And I would be 100% more than happy to pick up the deeper items that you've shared about the doc rework, as well as improving the documentation quality.

I might end up opening an issue in the next few days on one of the items where I feel that I can add value. Happy to be here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! What about helping with #21138 or with #19201?

You can also look through the help wanted tag on the issues to see if there is something you like.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They look great, happy to pick both one by one - #21138 would require a deeper scope and flow so maybe I can pick that one up first.

I'll leave a comment on that so we can just scope it out once. let me know if that sounds good

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that's the way to go about that. 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

awesome did that

shivamchhuneja and others added 2 commits June 3, 2025 19:18
Co-authored-by: Stefanie Senger <91849487+StefanieSenger@users.noreply.github.com>
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.

That looks very good. Thank you again for your work, @shivamchhuneja.

@adrinjalali, do you think we can merge that?

@betatim
Copy link
Member

betatim commented Jun 4, 2025

I'm not Adrin, but pressing merge anyway :D

@betatim betatim merged commit c4a0043 into scikit-learn:main Jun 4, 2025
36 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