[R-package] Updated lgb.Booster.R with keyword arguments #3496

Pey-crypto · 2020-10-28T02:03:19Z

-I am facing problems while generating the docs
Little help please
#3390

-I am facing problems while generating the docs Little help please

ghost · 2020-10-28T02:03:33Z

All CLA requirements met.

jameslamb

thanks for taking the time to contribute to {lightgbm}!

There are other function calls in lgb.Booster.R that also should be using keyword arguments.

Can you please add keyword arguments to these calls in this file?

lgb.check.r6.class()
lgb.params2str()
private$inner_eval()
lgb.call.return.str()
predictor$predict()
Predictor$new()
self$save_model_to_string()
object$predict()
booster$save_model()
booster$dump_model()

Once you've made these changes, I can re-generate the documentation and push it to this pull request.

jameslamb · 2020-10-28T02:47:22Z

R-package/R/lgb.Booster.R

@@ -335,7 +335,7 @@ Booster <- R6::R6Class(

      lower_bound <- 0.0
      lgb.call(
-        "LGBM_BoosterGetLowerBoundValue_R"
+        fun_name="LGBM_BoosterGetLowerBoundValue_R"


Suggested change

fun_name="LGBM_BoosterGetLowerBoundValue_R"

fun_name = "LGBM_BoosterGetLowerBoundValue_R"

Can you please put a space between the argument name and value for all of these cases?

Okay..will be done soon

jameslamb

thanks! I left a few small suggestions

jameslamb · 2020-10-29T00:45:55Z

R-package/R/lgb.Booster.R

+      private$inner_eval(
+        name = name,
+        data_idx = data_idx,
+        feval = feval)


Suggested change

private$inner_eval(

name = name,

data_idx = data_idx,

feval = feval)

private$inner_eval(

data_name = name

, data_idx = data_idx

, feval = feval

)

like I mentioned in #3391 , could you please use comma-first style when breaking up lines?

I think accepting this and the other suggestions will fix the test failures on GitHub Actions. The right keyword argument here is data_name, not name

jameslamb · 2020-10-29T00:58:32Z

R-package/R/lgb.Booster.R

+      predictor$predict(
+        data = data, 
+        start_iteration = start_iteration, 
+        num_iteration = num_iteration, 
+        rawscore = rawscore,
+        predleaf = predleaf, 
+        predcontrib = predcontrib, 
+        header = header, 
+        reshape = reshape)


Suggested change

predictor$predict(

data = data,

start_iteration = start_iteration,

num_iteration = num_iteration,

rawscore = rawscore,

predleaf = predleaf,

predcontrib = predcontrib,

header = header,

reshape = reshape)

predictor$predict(

data = data

, start_iteration = start_iteration

, num_iteration = num_iteration

, rawscore = rawscore

, predleaf = predleaf

, predcontrib = predcontrib

, header = header

, reshape = reshape

)

jameslamb · 2020-10-29T00:59:50Z

R-package/R/lgb.Booster.R

+  invisible(booster$save_model(
+    filename = filename, 
+    num_iterations = num_iteration))


Suggested change

invisible(booster$save_model(

filename = filename,

num_iterations = num_iteration))

invisible(booster$save_model(

filename = filename

, num_iteration = num_iteration

))

jameslamb · 2020-10-29T01:15:01Z

R-package/R/lgb.Booster.R

+      predictor$predict(
+        data = data, 
+        start_iteration = start_iteration, 
+        num_iteration = num_iteration, 
+        rawscore = rawscore,
+        predleaf = predleaf, 
+        predcontrib = predcontrib, 
+        header = header, 
+        reshape = reshape
+        )


Suggested change

predictor$predict(

data = data,

start_iteration = start_iteration,

num_iteration = num_iteration,

rawscore = rawscore,

predleaf = predleaf,

predcontrib = predcontrib,

header = header,

reshape = reshape

)

predictor$predict(

data = data

, start_iteration = start_iteration

, num_iteration = num_iteration

, rawscore = rawscore

, predleaf = predleaf

, predcontrib = predcontrib

, header = header

, reshape = reshape

)

jameslamb · 2020-10-29T01:49:50Z

R-package/R/lgb.Booster.R

-  invisible(booster$save_model(filename, num_iteration))
+  invisible(booster$save_model(
+    filename = filename
+    , num_iterations = num_iteration))


Suggested change

, num_iterations = num_iteration))

, num_iterations = num_iteration

))

jameslamb · 2020-10-29T01:50:11Z

R-package/R/lgb.Booster.R

-    , predcontrib
-    , header
-    , reshape
+    data = data


Suggested change

data = data

data = data

jameslamb · 2020-10-29T01:50:49Z

R-package/R/lgb.Booster.R

+      private$inner_eval(
+        data_name = name
+         , data_idx = data_idx
+         , feval = feval)


Suggested change

, feval = feval)

, feval = feval

)

jameslamb

sorry for all of the requested style changes. If you just click on the suggestions in the browser instead of committing them yourself, it might be easier

R-package/R/lgb.Booster.R

Co-authored-by: James Lamb <jaylamb20@gmail.com>

jameslamb

Thanks for the efforts so far! Please see my suggested changes.

I'll review again once this PR also adds the keywords args for these:

self$save_model_to_string()
lgb.check.r6.class()
Predictor$new()

R-package/R/lgb.Booster.R

Co-authored-by: James Lamb <jaylamb20@gmail.com>

This reverts commit 9b81ce8.

R-package/R/lgb.Booster.R

Co-authored-by: James Lamb <jaylamb20@gmail.com>

jameslamb

looks good so far! But you are still missing these:

self$save_model_to_string()
lgb.check.r6.class()
Predictor$new()

#3496 (review)

If you're struggling with them, please let me know and I'd be happy to help!

Pey-crypto · 2020-10-30T02:27:44Z

Not sure about these
-Predictor$new
-self$save_model_to_string

R-package/R/lgb.Booster.R

jameslamb · 2020-10-30T16:24:09Z

Not sure about these
-Predictor$new
-self$save_model_to_string

no problem! You can skip these, I will do them in a separate PR.

Co-authored-by: James Lamb <jaylamb20@gmail.com>

This reverts commit 4f2cc94.

jameslamb · 2020-10-30T16:30:04Z

R-package/R/lgb.Booster.R

@@ -216,7 +216,7 @@ Booster <- R6::R6Class(
      if (!is.null(train_set)) {

        # Check if training set is lgb.Dataset
-        if (!lgb.check.r6.class(train_set, "lgb.Dataset")) {
+       if (!lgb.check.r6.class(object = train_set, name = "lgb.Dataset"))


Suggested change

if (!lgb.check.r6.class(object = train_set, name = "lgb.Dataset"))

if (!lgb.check.r6.class(object = train_set, name = "lgb.Dataset")) {

It looks like this { was lost, and the lines are no longer aligned.

jameslamb · 2020-10-30T16:30:25Z

R-package/R/lgb.Booster.R

@@ -346,7 +346,7 @@ Booster <- R6::R6Class(
    eval = function(data, name, feval = NULL) {

      # Check if dataset is lgb.Dataset
-      if (!lgb.check.r6.class(data, "lgb.Dataset")) {
+      if (!lgb.check.r6.class(object = data, name = "lgb.Dataset"))


Suggested change

if (!lgb.check.r6.class(object = data, name = "lgb.Dataset"))

if (!lgb.check.r6.class(object = data, name = "lgb.Dataset")) {

It looks like the { was lost

jameslamb · 2020-10-30T16:32:10Z

R-package/R/lgb.Booster.R

@@ -149,7 +149,7 @@ Booster <- R6::R6Class(
    add_valid = function(data, name) {

      # Check if data is lgb.Dataset
-      if (!lgb.check.r6.class(data, "lgb.Dataset")) {
+      if (!(lgb.Dataset = lgb.check.r6.class(data, "lgb.Dataset"))) {


this check is still incorrect. Please accept this suggestion in your browser.

Suggested change

if (!(lgb.Dataset = lgb.check.r6.class(data, "lgb.Dataset"))) {

if (!lgb.check.r6.class(object = data, name = "lgb.Dataset")) {

…atch

R-package/R/lgb.Booster.R

Co-authored-by: James Lamb <jaylamb20@gmail.com>

R-package/R/lgb.Booster.R

Co-authored-by: James Lamb <jaylamb20@gmail.com>

jameslamb

thanks for your time and efforts! I will merge this once the builds are done.

github-actions · 2023-08-24T04:39:15Z

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.

Updated lgb.Booster.R with keyword arguments

aad38bb

-I am facing problems while generating the docs Little help please

Pey-crypto requested review from jameslamb and Laurae2 as code owners October 28, 2020 02:03

jameslamb requested changes Oct 28, 2020

View reviewed changes

jameslamb changed the title ~~Updated lgb.Booster.R with keyword arguments~~ [R-package] Updated lgb.Booster.R with keyword arguments Oct 28, 2020

jameslamb added the maintenance label Oct 28, 2020

Pey-crypto added 2 commits October 29, 2020 05:21

Update lgb.Booster.R

00f0524

Update lgb.Booster.R

8ab9fe5

jameslamb requested changes Oct 29, 2020

View reviewed changes

Pey-crypto added 2 commits October 29, 2020 06:50

Clean up as requested

07ac370

Update lgb.Booster.R

219b646

jameslamb requested changes Oct 29, 2020

View reviewed changes

Update lgb.Booster.R

b6d1185

jameslamb requested changes Oct 29, 2020

View reviewed changes

R-package/R/lgb.Booster.R Outdated Show resolved Hide resolved

R-package/R/lgb.Booster.R Outdated Show resolved Hide resolved

R-package/R/lgb.Booster.R Outdated Show resolved Hide resolved

Pey-crypto and others added 3 commits October 29, 2020 07:31

Update R-package/R/lgb.Booster.R

49ddde1

Co-authored-by: James Lamb <jaylamb20@gmail.com>

Update R-package/R/lgb.Booster.R

c629688

Co-authored-by: James Lamb <jaylamb20@gmail.com>

Update R-package/R/lgb.Booster.R

81a2e60

Co-authored-by: James Lamb <jaylamb20@gmail.com>

jameslamb self-requested a review October 29, 2020 05:24

jameslamb requested changes Oct 29, 2020

View reviewed changes

R-package/R/lgb.Booster.R Outdated Show resolved Hide resolved

Pey-crypto and others added 4 commits October 29, 2020 11:01

Update R-package/R/lgb.Booster.R

3a88050

Co-authored-by: James Lamb <jaylamb20@gmail.com>

Update lgb.Booster.R

9b81ce8

Update lgb.Booster.R

46fca40

Revert "Update lgb.Booster.R"

9929cae

This reverts commit 9b81ce8.

jameslamb requested changes Oct 29, 2020

View reviewed changes

R-package/R/lgb.Booster.R Outdated Show resolved Hide resolved

Update R-package/R/lgb.Booster.R

0e88211

Co-authored-by: James Lamb <jaylamb20@gmail.com>

jameslamb self-requested a review October 29, 2020 17:01

jameslamb requested changes Oct 29, 2020

View reviewed changes

Update lgb.Booster.R

1667eca

Pey-crypto added 4 commits October 30, 2020 06:58

Update lgb.Booster.R

e558aff

Update lgb.Booster.R

4f2cc94

Update lgb.Booster.R

04254c9

Update lgb.Booster.R

621c9fb

jameslamb requested changes Oct 30, 2020

View reviewed changes

R-package/R/lgb.Booster.R Outdated Show resolved Hide resolved

R-package/R/lgb.Booster.R Outdated Show resolved Hide resolved

R-package/R/lgb.Booster.R Outdated Show resolved Hide resolved

Pey-crypto and others added 4 commits October 30, 2020 21:54

Update R-package/R/lgb.Booster.R

c98d527

Co-authored-by: James Lamb <jaylamb20@gmail.com>

Update R-package/R/lgb.Booster.R

7037e82

Co-authored-by: James Lamb <jaylamb20@gmail.com>

Update R-package/R/lgb.Booster.R

69cc5a1

Co-authored-by: James Lamb <jaylamb20@gmail.com>

Revert "Update lgb.Booster.R"

f2be0d8

This reverts commit 4f2cc94.

jameslamb requested changes Oct 30, 2020

View reviewed changes

Pey-crypto added 2 commits October 30, 2020 22:10

Update lgb.Booster.R

d3fd5e3

Merge branch 'patch' of https://github.com/Pey-crypto/LightGBM into p…

0cedcd7

…atch

jameslamb requested changes Oct 30, 2020

View reviewed changes

R-package/R/lgb.Booster.R Outdated Show resolved Hide resolved

Update R-package/R/lgb.Booster.R

40708da

Co-authored-by: James Lamb <jaylamb20@gmail.com>

jameslamb self-requested a review October 30, 2020 17:21

Update lgb.Booster.R

e84a31a

jameslamb requested changes Oct 30, 2020

View reviewed changes

R-package/R/lgb.Booster.R Outdated Show resolved Hide resolved

R-package/R/lgb.Booster.R Outdated Show resolved Hide resolved

R-package/R/lgb.Booster.R Outdated Show resolved Hide resolved

Pey-crypto and others added 3 commits October 30, 2020 22:55

Update R-package/R/lgb.Booster.R

028b3f6

Co-authored-by: James Lamb <jaylamb20@gmail.com>

Update R-package/R/lgb.Booster.R

986906d

Co-authored-by: James Lamb <jaylamb20@gmail.com>

Update R-package/R/lgb.Booster.R

14e8ebe

Co-authored-by: James Lamb <jaylamb20@gmail.com>

jameslamb approved these changes Oct 30, 2020

View reviewed changes

Update lgb.Booster.R

6a944c4

jameslamb merged commit f7c1a7f into microsoft:master Oct 30, 2020

jameslamb mentioned this pull request Nov 28, 2020

[R-package] use keyword arguments for all internal function calls #3390

Closed

github-actions bot locked as resolved and limited conversation to collaborators Aug 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[R-package] Updated lgb.Booster.R with keyword arguments #3496

[R-package] Updated lgb.Booster.R with keyword arguments #3496

Pey-crypto commented Oct 28, 2020 •

edited

ghost commented Oct 28, 2020 •

edited by ghost

jameslamb left a comment

jameslamb Oct 28, 2020

Pey-crypto Oct 28, 2020

jameslamb left a comment

jameslamb Oct 29, 2020 •

edited

jameslamb Oct 29, 2020

jameslamb Oct 29, 2020

jameslamb Oct 29, 2020

jameslamb Oct 29, 2020

jameslamb Oct 29, 2020

jameslamb Oct 29, 2020

jameslamb Oct 29, 2020

jameslamb Oct 29, 2020

jameslamb left a comment

jameslamb left a comment

jameslamb left a comment •

edited

Pey-crypto commented Oct 30, 2020

jameslamb commented Oct 30, 2020

jameslamb Oct 30, 2020

jameslamb Oct 30, 2020

jameslamb Oct 30, 2020

jameslamb left a comment

github-actions bot commented Aug 24, 2023

	fun_name="LGBM_BoosterGetLowerBoundValue_R"
	fun_name = "LGBM_BoosterGetLowerBoundValue_R"

	, num_iterations = num_iteration))
	, num_iterations = num_iteration
	))

	if (!lgb.check.r6.class(object = train_set, name = "lgb.Dataset"))
	if (!lgb.check.r6.class(object = train_set, name = "lgb.Dataset")) {

	if (!lgb.check.r6.class(object = data, name = "lgb.Dataset"))
	if (!lgb.check.r6.class(object = data, name = "lgb.Dataset")) {

	if (!(lgb.Dataset = lgb.check.r6.class(data, "lgb.Dataset"))) {
	if (!lgb.check.r6.class(object = data, name = "lgb.Dataset")) {

[R-package] Updated lgb.Booster.R with keyword arguments #3496

[R-package] Updated lgb.Booster.R with keyword arguments #3496

Conversation

Pey-crypto commented Oct 28, 2020 • edited

ghost commented Oct 28, 2020 • edited by ghost

jameslamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jameslamb left a comment

Choose a reason for hiding this comment

jameslamb Oct 29, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jameslamb left a comment

Choose a reason for hiding this comment

jameslamb left a comment

Choose a reason for hiding this comment

jameslamb left a comment • edited

Choose a reason for hiding this comment

Pey-crypto commented Oct 30, 2020

jameslamb commented Oct 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jameslamb left a comment

Choose a reason for hiding this comment

github-actions bot commented Aug 24, 2023

Pey-crypto commented Oct 28, 2020 •

edited

ghost commented Oct 28, 2020 •

edited by ghost

jameslamb Oct 29, 2020 •

edited

jameslamb left a comment •

edited