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

Fix Integer Overflow #2357 #2400

Merged
merged 6 commits into from
Sep 27, 2019
Merged

Fix Integer Overflow #2357 #2400

merged 6 commits into from
Sep 27, 2019

Conversation

chris-smith-zocdoc
Copy link
Contributor

Fixes #2357

Please let me know if this is what you meant in your comment @guolinke

Where should I add tests for this?

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.

Please use two-space indent, not 4.

@StrikerRUS StrikerRUS requested a review from guolinke September 11, 2019 19:14
@chris-smith-zocdoc
Copy link
Contributor Author

Please use two-space indent, not 4.

Fixed. We could add an https://editorconfig.org/ file to automatically configure editor settings for people who use it. Example

@StrikerRUS
Copy link
Collaborator

How about simple explicit prohibition of large max_depth values in config as per #2357 (comment)?

// check = <=16

// desc = limit the max depth for tree model. This is used to deal with over-fitting when ``#data`` is small. Tree still grows leaf-wise
// desc = ``<= 0`` means no limit
int max_depth = -1;

@StrikerRUS
Copy link
Collaborator

StrikerRUS commented Sep 11, 2019

We could add an https://editorconfig.org/ file to automatically configure editor settings for people who use it. Example

Thanks a lot! Cool feature! I'm adding this to our feature requests (#2302) near the cpp lint test request.

Refer to #2401.

@StrikerRUS StrikerRUS mentioned this pull request Sep 11, 2019
@chris-smith-zocdoc
Copy link
Contributor Author

How about simple explicit prohibition of large max_depth values in config as per #2357 (comment)?

// check = <=16

// desc = limit the max depth for tree model. This is used to deal with over-fitting when ``#data`` is small. Tree still grows leaf-wise
// desc = ``<= 0`` means no limit
int max_depth = -1;

I'm not super familiar with tuning LightGBM yet, but I don't think we want to limit max_depth to such a small value. In one of my datasets I use a value around 40 and wouldn't be surprised if other people are also using a large value.

I would also consider this a significant change and should be discussed outside the scope of this bug fix.

@guolinke
Copy link
Collaborator

@StrikerRUS
I agree with @chris-smith-zocdoc , for leave-wise tree, max_depth could be large.

@chris-smith-zocdoc
Copy link
Contributor Author

@guolinke @StrikerRUS Would you like anything else added to this pr? Should I add tests or update docs?

@guolinke
Copy link
Collaborator

ping @StrikerRUS for the test and doc.

@StrikerRUS
Copy link
Collaborator

@chris-smith-zocdoc @guolinke I think that new fatal error message is quite clear and no clarifications in docs are needed.

@chris-smith-zocdoc
Copy link
Contributor Author

Thanks @StrikerRUS I think this is ready to merge then.

src/io/config.cpp Outdated Show resolved Hide resolved
src/io/config.cpp Outdated Show resolved Hide resolved
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.

@chris-smith-zocdoc Thanks a lot!

@StrikerRUS
Copy link
Collaborator

@guolinke Can we merge this?

@guolinke guolinke merged commit 1f88721 into microsoft:master Sep 27, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Mar 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

lgb.cv is constantly crashing in R
3 participants