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] [python-package] deprecate Dataset arguments to cv() and train() #6446

Merged
merged 6 commits into from
May 11, 2024

Conversation

jameslamb
Copy link
Collaborator

Contributes to #6435.

Adds deprecation warnings for all the cases mentioned in #6435, and some other changes that flow from that:

  • promotes categorical_feature and colnames to top-level arguments of lightgbm() in the R package
    • so it stops relying on lgb.train()'s support for setting those on the Dataset
  • modifies examples and tests to no longer use the deprecated pattern
  • updates relevant docs

Notes for Reviewers

I'd like to get these warnings out in the v4.4.0 release (#6439), and then leave them there for at least 2 more releases.

copying some others who may be interested: @mayer79 @david-cortes @simonpcouch

How I tested this

Ran the R tests and Python tests, and checked for these warnings.

# R
sh build-cran-package.sh --no-build-vignettes
R CMD INSTALL ./lightgbm_4.3.0.99.tar.gz
cd R-package/tests
Rscript testthat.R
cd ../../

# python
cmake -B build -S .
cmake --build build --target _lightgbm -j4
sh build-python.sh install --precompile
pytest tests/python_package_test

Interactively ran some code in both languages and confirmed that the expected warnings were raised.

Copy link
Collaborator

@borchero borchero left a comment

Choose a reason for hiding this comment

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

Nice, thanks a lot!

@jameslamb
Copy link
Collaborator Author

@jmoralez what do you think about this?

@jmoralez
Copy link
Collaborator

Hey. Sorry, I had given this a thumbs up but I guess my internet decided against it. I'm in favor of this, it'll reduce the complexity of the code and also avoid confusion on which place has the highest priority, etc.

@jameslamb
Copy link
Collaborator Author

Great, thank you! I'm very glad someone asked about this on Stack Overflow... I've been staring at this code for 7 years and never noticed the duplication 😅

@jameslamb jameslamb merged commit a70e832 into master May 11, 2024
38 checks passed
@jameslamb jameslamb deleted the deprecate-args branch May 11, 2024 00:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants