-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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] add reset_data in lgb.cv #1874
Conversation
R-package/R/lgb.cv.R
Outdated
@@ -303,7 +305,9 @@ lgb.cv <- function(params = list(), | |||
if (env$met_early_stop) break | |||
|
|||
} | |||
|
|||
if (free_boosters) { | |||
cv_booster$boosters = NULL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Laurae2 is this enough to free the memories of boosters?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@guolinke would anyone ever NOT want to free them? I'd rather just make this new behavior and leave the function signature the same. Adding flags to the code makes it harder to maintain, in my experience.
@jameslamb @Laurae2 |
@@ -12,6 +12,8 @@ | |||
#' @param categorical_feature list of str or int | |||
#' type int represents index, | |||
#' type str represents feature names | |||
#' @param callbacks list of callback functions | |||
#' List of callback functions that are applied at each iteration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how is this change related?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find the doc of this parameter is misning, compared with lgb.cv.
|
||
if (reset_data) { | ||
lapply(cv_booster$boosters, function(fd) { | ||
# Store temporarily model data elsewhere |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to me
fix #1854