[python] [R-package] Use the same address when updated label/weight/query #2662
Conversation
|
better fix (todo): use metadata.label/weight in objective and metric. |
… during training.
guolinke
requested review from
henry0312,
jameslamb,
Laurae2,
StrikerRUS and
wxchan
as code owners
| @@ -207,6 +208,12 @@ Booster <- R6::R6Class( | |||
| # Perform boosting update iteration | |||
| update = function(train_set = NULL, fobj = NULL) { | |||
|
|
|||
| if (is.null(train_set)) { | |||
| if (private$train_set$.__enclos_env__$private$version != private$train_set_version) { | |||
There was a problem hiding this comment.
Instead of x != y, could you use !identical(x, y) here? The issue is that having a NULLL` on either side of a logical like this in R will cause an error.
There was a problem hiding this comment.
given there are two integers, do we still need to use identical?
There was a problem hiding this comment.
I wasn't sure if they were guaranteed to always be non-null. You can use != if they will always be integers.
There was a problem hiding this comment.
yeah, if there is a NULL, it means there are bugs in the code.
| @@ -55,6 +55,7 @@ Booster <- R6::R6Class( | |||
|
|
|||
| # Create private booster information | |||
| private$train_set <- train_set | |||
| private$train_set_version <- train_set$.__enclos_env__$private$version | |||
There was a problem hiding this comment.
Maybe a naive question, but what does it mean to have a new "version" of the training set? Is it mainly updating weights as part of boosting? I think I am missing some context, so I don't understand what problem this patch solves.
There was a problem hiding this comment.
currently, Dataset is allowed to update label/weight/... (field data) during training, but the booster cannot capture these changes. And ideally, we should update related classes, such as the cached label/weight/.. in Objective/Metric. So I add a version number to indicate the changes of these fields.
There was a problem hiding this comment.
Ah I see ok, makes sense. Thanks!
jameslamb
changed the title
No description provided.