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

Max bin by feature #2190

Merged
merged 7 commits into from Jul 8, 2019
Merged

Max bin by feature #2190

merged 7 commits into from Jul 8, 2019

Conversation

btrotta
Copy link
Collaborator

@btrotta btrotta commented May 25, 2019

Implement feature requested in issue #1921. Add new parameter max_bin_by_feature.

@StrikerRUS StrikerRUS requested a review from guolinke May 25, 2019 13:34
StrikerRUS
StrikerRUS reviewed May 27, 2019
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.

Let me put my two cents in it :-)

src/io/dataset_loader.cpp Outdated Show resolved Hide resolved
src/io/dataset_loader.cpp Outdated Show resolved Hide resolved
src/io/dataset_loader.cpp Outdated Show resolved Hide resolved
src/io/dataset_loader.cpp Outdated Show resolved Hide resolved
src/io/dataset_loader.cpp Outdated Show resolved Hide resolved
src/io/dataset_loader.cpp Outdated Show resolved Hide resolved
src/io/dataset_loader.cpp 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
tests/python_package_test/test_engine.py Outdated Show resolved Hide resolved
@btrotta
Copy link
Collaborator Author

btrotta commented May 28, 2019

@StrikerRUS Fixed, thanks!
Is there any convention about line length? I noticed the Google style guide says 80 characters, but there are many longer lines in the existing code.

@StrikerRUS
Copy link
Collaborator

StrikerRUS commented May 28, 2019 •
edited

@btrotta Thanks a lot!
Speaking about line length, we, as many other modern projects, ignore 80 chars limit. I personally try to keep line length which do not cause horizontal scroll on GitHub 😃

@StrikerRUS
Copy link
Collaborator

@guolinke Should it be in 2.2.4 release?

@guolinke
Copy link
Collaborator

Yeah, it can be.

@StrikerRUS StrikerRUS requested a review from chivee June 18, 2019 03:46
@chivee chivee mentioned this pull request Jun 20, 2019
Merged
@StrikerRUS
Copy link
Collaborator

@chivee Can you please give it a second review?

@nirupamkar
Copy link

is max_bin_by_feature ready to use?

@StrikerRUS
Copy link
Collaborator

@nirupamkar

is max_bin_by_feature ready to use?

I think so, you can try this branch.

@guolinke guolinke merged commit 291752d into microsoft:master Jul 8, 2019
@StrikerRUS StrikerRUS mentioned this pull request Jul 8, 2019
Closed
@nirupamkar
Copy link

@StrikerRUS does max_bin_by_feature overwrite max_cat_thresold ?

@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.
Reviewers

@StrikerRUS StrikerRUS StrikerRUS left review comments

@guolinke guolinke guolinke approved these changes

@chivee chivee Awaiting requested review from chivee

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

Successfully merging this pull request may close these issues.

None yet
4 participants
@btrotta @StrikerRUS @guolinke @nirupamkar