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] Update remaining internal function calls to use keyword arguments #3617

Merged
merged 7 commits into from
Dec 1, 2020
Merged

[R-package] Update remaining internal function calls to use keyword arguments #3617

merged 7 commits into from
Dec 1, 2020

Conversation

zenggyu
Copy link
Contributor

@zenggyu zenggyu commented Dec 1, 2020

Closes #3390 .

I didn't edit the following .R files because all internal function calls in these files are already using keyword arguments:

  • lgb.plot.importance.R
  • lgb.unloader.R
  • metrics.R
  • readRDS.lgb.Booster.R
  • saveRDS.lgb.Booster.R

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 doing this! These changes all look great. Could you please also add keyword arguments for these other calls?

Let me know if you get stuck and need help, some of these are R6 objects way nested in the attributes of other R6 objects.

lgb.Dataset.R

  • self$set_categorical_feature()
  • self$set_colnames()
  • private$set_predictor()
  • x$set_colnames()
  • lgb.is.Dataset()
  • dataset$set_categorical_feature()
  • dataset$set_reference()
  • dataset$save_binary()

lgb.convert_with_rules.R

  • .get_column_classes()

lgb.cv.R

  • Predictor$new()
  • lgb.check_interaction_constraints()
  • data$update_params()
  • data$.__enclos_env__$private$set_predictor()
  • data$set_colnames()
  • data$set_categorical_feature()
  • categorize.callbacks()

@zenggyu
Copy link
Contributor Author

zenggyu commented Dec 1, 2020

OK!

@zenggyu
Copy link
Contributor Author

zenggyu commented Dec 1, 2020

So I used regular expression to find all functions defined in .R files, then used those function names to find all function calls and inspected them one by one. I have updated almost every function call which uses positional arguments instead of keyword arguments. There is one exception though. In lgb.cv.R and lgb.train.R, there are these function calls:

for (f in cb$pre_iter) {
   f(env)
}

I think f() comes from categorize.callbacks() in the callback.R file, and its one and only argument is x. However, I am not sure. Is this correct? Do you want me to update these function calls as well?

@jameslamb jameslamb self-requested a review December 1, 2020 15:51
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.

This looks great, thanks so much! You can ignore the f(env) things in callbacks.R.

The only CI job that's failing is the check-docs task on Travis, which isn't related to your changes. It just fails sometimes. I manually restarted it. Once that succeeds, I'll merge this.

Thanks for all the hard work! Come back and contribute any time!

@jameslamb jameslamb merged commit ab0d71d into microsoft:master Dec 1, 2020
@zenggyu zenggyu deleted the keyword_arguments branch January 5, 2021 01:02
@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.

[R-package] use keyword arguments for all internal function calls
2 participants