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

Cleanup MissingType enum constants #2931

Merged
merged 2 commits into from
Apr 2, 2020

Conversation

AlbertoEAF
Copy link
Contributor

@AlbertoEAF AlbertoEAF commented Mar 21, 2020

  • Clean up comparisons of missing_type using the enum name instead of hard-coded constants

    This does not incur in any overhead and eases code comprehension.

@AlbertoEAF
Copy link
Contributor Author

P.S.: I placed this PR whilst trying to understand the code in #2921.

src/io/tree.cpp Outdated Show resolved Hide resolved
@AlbertoEAF
Copy link
Contributor Author

@guolinke, do you approve this refactor? :)

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

src/io/tree.cpp Outdated Show resolved Hide resolved
src/io/tree.cpp Outdated Show resolved Hide resolved
@jameslamb
Copy link
Collaborator

If you rebase to master, to get the changes from #2954 , the R test issues and linting issues should be resolved. That will also fix any weirdness in the Travis build, which was a result of the recent Travis outage.

Sorry for the inconvenience!

@AlbertoEAF
Copy link
Contributor Author

Done @jameslamb @guolinke :)

@jameslamb
Copy link
Collaborator

Done @jameslamb @guolinke :)

Thank you! The Travis failures left (on check-docs) task are unrelated to this PR and being fixed in #2956 . @guolinke if you are ok with the current state of this PR (it's been a few days since your approval), you can use your admin power to merge this despite failing CI since all other checks except check-docs are passing.

@StrikerRUS
Copy link
Collaborator

The Travis failures left (on check-docs) task are unrelated to this PR and being fixed in #2956 .

Merged now.

@AlbertoEAF
Copy link
Contributor Author

I think it was approved but not merged @StrikerRUS ?

@jameslamb
Copy link
Collaborator

I think it was approved but not merged @StrikerRUS ?

it was for sure merged

image

@AlbertoEAF
Copy link
Contributor Author

AlbertoEAF commented Mar 30, 2020

Sorry, I misunderstood, thought you were talking about this merge, not #2956 :)

@AlbertoEAF
Copy link
Contributor Author

Can this be approved @guolinke ?

@StrikerRUS
Copy link
Collaborator

@AlbertoEAF Please rebase to master. It will allow us to merge this PR.

@AlbertoEAF
Copy link
Contributor Author

Done @StrikerRUS :)

@StrikerRUS
Copy link
Collaborator

@AlbertoEAF Thank you very much for your help!

@StrikerRUS StrikerRUS changed the title [refactor] Cleanup MissingType enum constants Cleanup MissingType enum constants Apr 2, 2020
@StrikerRUS StrikerRUS merged commit 51f37e9 into microsoft:master Apr 2, 2020
@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 24, 2023
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.

4 participants