-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
fix test_monotone_constraints often fails on MPI builds #3683
fix test_monotone_constraints often fails on MPI builds #3683
Conversation
best_left_constraints.min > best_left_constraints.max) { | ||
continue; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could fix the indent?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am sorry I am no sure what you mean. Is the indent here not the same way it is in the other "if" in the code, e.g. line 340?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@CharlesAuguste I guess @guolinke meant the following indent:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see it now, and fixed it, sorry about that!
@CharlesAuguste Thank you very much for the fix! |
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. |
This is a PR to fix issue #3621. The thing was I didn't take into account the case where different constraints on different thresholds would prevent a split from happening. So, very rarely, a bad split happened which made the test fail. In theory this could have broken the monotone constraints required by the end user, but it happens so rarely that I don't think the end users have really been impacted (if it happens on a single tree in a forest of 100 trees, it most likely won't show). In this PR I make sure that constraints are admissible for any split, and I think that should remove the bug @StrikerRUS reported. Let me know what you think.
As an example, the following piece of code was producing the error mentioned in #3621, but runs smoothly now.