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

renamed cur_cat => cur_cat_idx and added some comments #5522

Merged
merged 5 commits into from
Oct 11, 2022

Conversation

zyxue
Copy link
Contributor

@zyxue zyxue commented Oct 4, 2022

just for clarity and trying to make it easier to follow the code for those who aren't familiar with the code base.

@zyxue zyxue changed the title renamed cur_cat => cur_cat_index and added some comments renamed cur_cat => cur_cat_idx and added some comments Oct 4, 2022
src/io/bin.cpp Outdated Show resolved Hide resolved
Copy link
Collaborator

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

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

Thanks for the changes.

This looks ok to me, but I'll just leave a "comment" review and let @guolinke or @shiyu1994 check that this new name correctly reflects what the code is doing.

Until then... can you please merge in the latest state of master? We just merged a fix for the failing CI jobs (#5510). Sorry for the inconvenience.

Copy link
Collaborator

@guolinke guolinke left a comment

Choose a reason for hiding this comment

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

LGTM

@jameslamb
Copy link
Collaborator

@zyxue please push a new commit addressing the linting errors (build link).

src/io/bin.cpp:355:  At least two spaces is best between code and comments  [whitespace/comments] [2]
src/io/bin.cpp:392:  At least two spaces is best between code and comments  [whitespace/comments] [2]
src/io/bin.cpp:454:  At least two spaces is best between code and comments  [whitespace/comments] [2]

@jameslamb jameslamb self-requested a review October 11, 2022 14:32
Copy link
Collaborator

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

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

Looks good, thanks! We will merge this when it builds.

@jameslamb jameslamb merged commit c35ecfb into microsoft:master Oct 11, 2022
@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 19, 2023
@zyxue zyxue deleted the rename-in-find-bin branch August 20, 2023 15:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants