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] respect aliases for objective and metric and lgb.train() and lgb.cv() #4913

Merged
merged 6 commits into from
Jan 5, 2022

Conversation

jameslamb
Copy link
Collaborator

Similar to #4903, this PR proposes changes to ensure that parameter aliases in lgb.train() and lgb.cv() are handled correctly in the R package.

Currently, internal helper functions access params$objective and params$metric directly, without considering aliases.

if (is.character(params$objective)) {

if (is.null(params$metric)) {

As a result, it's not possible today to use any of the aliases for objective with lgb.train() and lgb.cv().

library(lightgbm)
data("agaricus.train", package = "lightgbm")
train <- agaricus.train
dtrain <- lgb.Dataset(
    data = train$data
    , label = train$label
)
bst <- lgb.train(
    data = dtrain
    , params = list(
        num_leaves = 5L
        , application = "binary"
        , num_iterations = 5L
    )
)

Error in lgb.check.obj(params = params, obj = obj) :
lgb.check.obj: objective should be a character or a function

This PR proposes the following changes to address that:

  • populate params$objective and params$metric using internal helper function lgb.check.wrapper_param(), which handles reconciling parameters passed by main parameter name and aliases
  • move code for handling the possibility of custom objective and metrics (which are R function objects, not strings) down so it happens after aliases have been handled in params

@StrikerRUS
Copy link
Collaborator

Could you please resolve a conflict with master?

Copy link
Collaborator

@StrikerRUS StrikerRUS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you!

expect_equal(sort(names(record_results)), c("auc", "binary_logloss"))
expect_length(record_results[["auc"]][["eval"]], nrounds)
expect_length(record_results[["binary_logloss"]][["eval"]], nrounds)
expect_equal(bst$params[["metric"]], list("auc", "binary_logloss"))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this assertion! Can we add the same assertion for objective into test_that("lgb.train() respects parameter aliases for objective", test?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah yep, definitely! good point that params should be checked for that test too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added in f90b2d6

@jameslamb jameslamb merged commit ac821f0 into master Jan 5, 2022
@jameslamb jameslamb deleted the r/metric-and-obj-aliases branch January 5, 2022 01:53
@StrikerRUS StrikerRUS mentioned this pull request Jan 6, 2022
13 tasks
@jameslamb jameslamb mentioned this pull request Oct 7, 2022
40 tasks
@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 23, 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.

2 participants