[R-package] Use keyword arguments with internal constructor calls

[jameslamb]

The classes in the R package all include calls that look similar to this:

private$rgf_init = RGF_mod$RGFRegressor(as.integer(max_leaf), as.integer(test_interval), algorithm, loss, reg_depth, l2, sl2, normalize,
                                                                         min_samples_leaf, n_iter, as.integer(n_tree_search), as.integer(opt_interval), learning_rate,
                                                                         memory_policy, as.integer(verbose))

That is 15 positional arguments. In my humble opinion, using this many positional arguments raises the risk of subtle and silent failures. I think explicit keyword arguments should be used in those calls. That will catch problems and have the side benefit of failing loudly in CI if the API for e.g. RGFRegressor changes.

[StrikerRUS]

I think it's good idea!

ping @mlampros

[mlampros]

@jameslamb would you mind give an example with explicit keyword arguments for this specific code snippet. thanks.

[jameslamb]

yes sorry! Should have given an example.

RGF_Regressor has this line at the end of its constructor:

# initialize RGF_Regressor
#------------------------
private$rgf_init <- RGF_mod$RGFRegressor(
    as.integer(max_leaf)
    , as.integer(test_interval)
    , algorithm
    , loss
    , reg_depth
    , l2
    , sl2
    , normalize
    , min_samples_leaf
    , n_iter
    , as.integer(n_tree_search)
    , as.integer(opt_interval)
    , learning_rate
    , memory_policy
    , as.integer(verbose)
)

I am proposing changing it to this:

private$rgf_init <- RGF_mod$RGFRegressor(
    max_leaf = as.integer(max_leaf)
    , test_interval = as.integer(test_interval)
    , algorithm = algorithm
    , loss = loss
    , reg_depth = reg_depth
    , l2 = l2
    , sl2 = sl2
    , normalize = normalize
    , min_samples_leaf = min_samples_leaf
    , n_iter = n_iter
    , n_tree_search = as.integer(n_tree_search)
    , opt_interval = as.integer(opt_interval)
    , learn_rate = learning_rate
    , memory_policy = memory_policy
    , verbose = as.integer(verbose)
)

Without keyword args, a PR that flipped the order of n_iter and n_tree_search in the RGFRegressor on the Python side would not know that it was silently breaking the R package. If that happened, you would be passing the value of n_iter to n_tree_search and vice versa. Since these are both integer valued, none of the underlying code would ever complain.

So basically the keyword idea is a free protection against the small possibility of something like my example above happening.

[mlampros]

@jameslamb you are right. You can proceed with the PR.

[jameslamb]

ok! will do