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.

Sign up for GitHub

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

Jump to bottom

enforce interaction constraints with monotone_constraints_method = intermediate/advanced #4043

Merged
merged 6 commits into from
Apr 11, 2021
Merged

enforce interaction constraints with monotone_constraints_method = intermediate/advanced #4043

merged 6 commits into from
Apr 11, 2021

Conversation

ChristophAymannsQC
Copy link
Contributor

@ChristophAymannsQC ChristophAymannsQC commented Mar 4, 2021 •
edited
Loading

First attempt to address #4044 .

@ghost
Copy link

ghost commented Mar 4, 2021 •
edited by ghost
Loading

CLA assistant check
All CLA requirements met.

@ChristophAymannsQC ChristophAymannsQC changed the title add test for interaction constraints and monotone constraints enforce interaction constraints with monotone_constraints_method = intermediate/advanced Mar 4, 2021
@ChristophAymannsQC ChristophAymannsQC mentioned this pull request Mar 4, 2021
Closed
@ChristophAymannsQC ChristophAymannsQC marked this pull request as ready for review March 5, 2021 09:01
btrotta
btrotta approved these changes Mar 6, 2021
View reviewed changes
Copy link
Collaborator

@btrotta btrotta left a comment

Choose a reason for hiding this comment

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

Looks fine to me

StrikerRUS
StrikerRUS requested changes Mar 7, 2021
View reviewed changes
Copy link
Collaborator

@StrikerRUS StrikerRUS left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your contribution!
I have only few minor comments for tests code.

tests/python_package_test/test_engine.py Outdated Show resolved Hide resolved
tests/python_package_test/test_engine.py Outdated Show resolved Hide resolved
tests/python_package_test/test_engine.py Outdated Show resolved Hide resolved
@ChristophAymannsQC @StrikerRUS
Apply suggestions from code review
88848d1 
Co-authored-by: Nikita Titov <nekit94-08@mail.ru>
@ChristophAymannsQC
Copy link
Contributor Author

Thanks a lot for your contribution!
I have only few minor comments for tests code.

Thanks a lot for the comments. Added as suggested.

StrikerRUS
StrikerRUS approved these changes Mar 8, 2021
View reviewed changes
Copy link
Collaborator

@StrikerRUS StrikerRUS left a comment

Choose a reason for hiding this comment

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

Python test part looks fine to me! Thank you!

jameslamb
jameslamb previously requested changes Mar 8, 2021
View reviewed changes
src/treelearner/serial_tree_learner.cpp Show resolved Hide resolved
@StrikerRUS
Copy link
Collaborator

I think @CharlesAuguste is interested in this.

@StrikerRUS StrikerRUS mentioned this pull request Mar 13, 2021
Merged
@StrikerRUS StrikerRUS added fix and removed feature labels Mar 17, 2021
@StrikerRUS
Copy link
Collaborator

Gently ping @CharlesAuguste

@CharlesAuguste
Copy link
Contributor

I completely missed this, I am very sorry. I will have a look at this PR in the next few days and write back.

@CharlesAuguste
Copy link
Contributor

@StrikerRUS I just had a look at the code, and I agree with @ChristophAymannsQC that everything should be working well with this fix. I also think it was a good idea to parametrize the test to keep the old test, because the test with the interaction constraints may not be able to catch some issues relating to monotone constraints.

@StrikerRUS
Copy link
Collaborator

@CharlesAuguste Thanks a lot for your feedback!

@ChristophAymannsQC Thank you very much!

@StrikerRUS StrikerRUS merged commit 9e1d7fa into microsoft:master Apr 11, 2021
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity since it was closed. To start a new related discussion, open a new issue at https://github.com/microsoft/LightGBM/issues including a reference to this.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@StrikerRUS StrikerRUS StrikerRUS approved these changes

@jameslamb jameslamb jameslamb left review comments

@btrotta btrotta btrotta approved these changes

@chivee chivee Awaiting requested review from chivee

@guolinke guolinke Awaiting requested review from guolinke guolinke is a code owner

@henry0312 henry0312 Awaiting requested review from henry0312

@wxchan wxchan Awaiting requested review from wxchan

Assignees
No one assigned
Labels
fix
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ChristophAymannsQC @StrikerRUS @CharlesAuguste @jameslamb @btrotta