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

[R-package] refactor and improvements to lgb.convert() functions (fixes #2678, #2681) #3269

Merged
merged 10 commits into from
Aug 6, 2020

Conversation

jameslamb
Copy link
Collaborator

This PR fixes #2678 and #2681 by adding the following improvements to lgb.convert() functions:

  • lgb.convert():
    • now fills in missing values in factor and character columns with 0
  • lgb.convert_with_rules():
    • rules no longer contains rules where the "name" in the rule is a missing value
  • both:
    • now converts logical columns to integer, filling in missing values with -1
    • now warns if any columns remain which are not numeric or integer
    • moved code for getting column classes into shared internal function to reduce duplication
    • changed from inherits(data, "data.table") to data.table::is.data.table()
    • moved error handling up to the beginning of the functions
    • clarified documentation

Question for reviewers

@Laurae2 @guolinke could we remove lgb.convert() and just offer lgb.convert_with_rules()? Using lgb.convert_with_rules() without passing your own rules (the default), has identical behavior to lgb.convert() and is better because it returns the rules on how columns were changed, which improves reproducibility. Since we're considering a 3.0.0 release in #3071 right now, I think this is the right time for such a breaking change.

@guolinke
Copy link
Collaborator

guolinke commented Aug 4, 2020

Yeah, i think it is great

@jameslamb
Copy link
Collaborator Author

Yeah, i think it is great

ok thanks! I can do that

@jameslamb
Copy link
Collaborator Author

I just removed lgb.convert() in db27920. I also removed the demo specific to that function from demo/.

@jameslamb jameslamb added breaking and removed feature labels Aug 5, 2020
@jameslamb jameslamb mentioned this pull request Aug 5, 2020
@StrikerRUS StrikerRUS removed their request for review August 5, 2020 17:44
@guolinke guolinke closed this Aug 5, 2020
@guolinke guolinke reopened this Aug 5, 2020
@jameslamb
Copy link
Collaborator Author

@guolinke I'll merge master into this, fix the conflicts, and merge it once CI passes. Hopefully should be quick.

@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.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[R-package] lgb.convert() functions should convert columns of type 'logical'
2 participants