Lightgbm: [R-package] use keyword arguments for all internal function calls

Created on 13 Sep 2020  路  6Comments  路  Source: microsoft/LightGBM

Summary

Today in the R package, there are a lot of internal function calls which use only positional arguments. Change them to use keyword arguments for extra safety.

I've added this issue to provide a small, focused contribution opportunity for Hacktoberfest 2020 participants. If you are an experienced open source contributor, please leave this for beginners and consider contributing on a different backlog item.

Motivation

Using positional arguments in internal code is dangerous, because it can allow mistakes to slip through or can lead to confusing errors.

This is especially dangerous for cases where the values you'd pass to two parameters take on similar values. For example, in lightgbm(), eval_freq and early_stopping_rounds are both positive integers and can take on similar values. You might not known, unless you're very careful, that you accidentally specified them in the wrong order.

How to Contribute

To help with this issue, propose a pull request which replaces uses of positional arguments in the R package with keyword arguments.

That means changing calls like this:

# before
folds <- generate.cv.folds(
    nfold
    , nrow(data)
    , stratified
    , getinfo(data, "label")
    , getinfo(data, "group")
    , params
)

# after
folds <- generate.cv.folds(
    nfold = nfold
    , nrows = nrow(data)
    , stratified = stratified
    , label = getinfo(data, "label")
    , group = getinfo(data, "group")
    , params = params
)
  1. Only change code in R-package/R/
  2. Before submitting your PR, re-generate the documentation. Be sure you have {roxygen2} 7.1.1 installed. If you have any issues with this, just ask when you submit your PR and I can regenerate the docs for you
    shell sh build-cran-package.sh R CMD INSTALL --with-keep.source lightgbm_3.*.tar.gz cd R-package Rscript -e "roxygen2::roxygenize(load = 'installed')"
  3. Do not modify any calls to lgb.call()
  4. Do not worry about any function calls from the standard library (like print(), length(), is.character(), etc.)
  5. If adding these arguments makes lines too long, use comma-first style with one argument per line
    r result <- some_function( arg1 = TRUE , arg2 = 4 , arg3 = "hello" , ... )

Refer to #3391 as an example. If you have any questions, tag me on this issue and I can help.

Assignments

If you are interested, please comment on this issue and indicate which file in R-package/R/ you'd like to help on. PLEASE ONLY TAKE ONE FILE, so that multiple people can use this as a learning experience.

This issue is not urgent, so you can claim a file now and not contribute until Hacktoberfest begins on October 1st.

The list below tracks the assignments and completions so far.

  • ~aliases.R~
  • ~callback.R~ - @philip-khor (#3430)
  • lgb.Booster.R - @Pey-crypto
  • lgb.Dataset.R
  • ~lgb.Predictor.R~ - @AnshuTrivedi (#3464)
  • lgb.convert_with_rules.R
  • lgb.cv.R
  • lgb.importance.R
  • lgb.interprete.R
  • lgb.model.dt.tree.R
  • lgb.plot.importance.R
  • lgb.plot.interpretation.R
  • ~lgb.train.R~ - @iadi7ya (#3452)
  • lgb.unloader.R
  • ~lightgbm.R~
  • metrics.R
  • readRDS.lgb.Booster.R
  • ~removed.R~
  • saveRDS.lgb.Booster.R
  • utils.R

Thanks for contributing to LightGBM!

good first issue r-package

All 6 comments

can I claim callback.R please?

Hi @jameslamb , I'm working on lgb.train.R. Thanks!

thanks @iadi7ya ! I appreciate you taking the time to contribute to {lightgbm}!

@jameslamb I am working on lgb.Predictor.R

can i claim lgb.Booster.R?

can i claim lgb.Booster.R?

yes please!

Was this page helpful?
0 / 5 - 0 ratings