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

[python-package] [R-package] include more params in model text representation (fixes #6010) #6077

Merged
merged 17 commits into from
Sep 13, 2023

Conversation

jameslamb
Copy link
Collaborator

@jameslamb jameslamb commented Sep 5, 2023

Fixes #6010.

Proposes adding the following parameters to the set written to model text files (and therefore also the set preserved by pickling/unpickled in Python and saveRDS() / readRDS() in R):

data_sample_strategy
num_grad_quant_bins
quant_train_renew_leaf
seed
stochastic_rounding
use_quantized_grad

It also refactors the include/config.h mechanism, to make the relationship between that file, src/config_auto.cpp, and the Parameter docs clearer.

Notes for Reviewers

This PR was mainly inspired by #6017 (comment), but it's informed by many prior conversations related to this topic:

@jameslamb jameslamb added the fix label Sep 5, 2023
@jameslamb jameslamb changed the title WIP: [python-package] [R-package] include all params in model text representation (fixes #6010) WIP: [python-package] [R-package] include more params in model text representation (fixes #6010) Sep 7, 2023
@jameslamb jameslamb changed the title WIP: [python-package] [R-package] include more params in model text representation (fixes #6010) [python-package] [R-package] include more params in model text representation (fixes #6010) Sep 7, 2023
Copy link
Collaborator

@jmoralez jmoralez left a comment

Choose a reason for hiding this comment

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

LGTM. Just a suggestion

R-package/tests/testthat/test_lgb.Booster.R Outdated Show resolved Hide resolved
R-package/tests/testthat/test_lgb.Booster.R Outdated Show resolved Hide resolved
stop(error_msg)
}
}
}
Copy link
Collaborator Author

@jameslamb jameslamb Sep 12, 2023

Choose a reason for hiding this comment

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

I tested both sides of this, like this:

.expect_in <- function(x, y) {
  if (exists("expect_in")) {
    expect_in(x, y)
  } else {
    missing_items <- x[!(x %in% y)]
    if (length(missing_items) != 0L) {
      error_msg <- paste0("Some expected items not found: ", toString(missing_items))
      stop(error_msg)
    }
  }
}
# testthat not loaded
.expect_in(c("a", "b"), c("d", "a", "a"))

Error in .expect_in(c("a", "b"), c("d", "a", "a")) :
Some expected items not found: b

# testthat loaded
library(testthat)
.expect_in(c("a", "b"), c("d", "a", "a"))

Error: x (actual) isn't fully contained within y (expected).

  • Missing from expected: "b"
  • Present in expected: "d", "a", "a"

@jmoralez jmoralez self-requested a review September 13, 2023 17:26
@jameslamb jameslamb merged commit ab1eaa8 into master Sep 13, 2023
41 checks passed
@jameslamb jameslamb deleted the include-all-model-params branch September 13, 2023 22:35
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:/microsoft/LightGBM/issues including a reference to this.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 13, 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.

[Docs] 4.0.0 parameters missing from model.txt
2 participants