WIP: [R-package] Add support for specifying training indices in lgb.cv()

[julioasotodv]

fixes #3924

[ghost]

All CLA requirements met.

[julioasotodv]

No idea why Azure pipelines are unhappy. It looks like it was an issue with the instances while building the containers... Shall we force rebuild?

[StrikerRUS]

@julioasotodv

It looks like it was an issue with the instances while building the containers... Shall we force rebuild?

Thanks, done!

[jameslamb]

Thanks for taking the time to contribute! This is a really nice addition.

Could you please add a unit test in the lgb.cv() section, checking that this behavior works as expected? It would be great if you can set up a small time-series cross validation example as the test. https://github.com/microsoft/LightGBM/blob/master/R-package/tests/testthat/test_basic.R#L262

[jameslamb]

I changed the PR's description to say "fixes" instead of "as seen in". That way when we merge this, the issue will be closed automatically.

[julioasotodv]

Thanks for taking the time to contribute! This is a really nice addition.

Could you please add a unit test in the lgb.cv() section, checking that this behavior works as expected? It would be great if you can set up a small time-series cross validation example as the test. https://github.com/microsoft/LightGBM/blob/master/R-package/tests/testthat/test_basic.R#L262

Sure! I will try to come up with a meaningful test.

[jameslamb]

@julioasotodv are you still interested in contributing this? I'm available to help if you have any questions.

[julioasotodv]

@jameslamb Indeed! Sorry had a couple of busy weeks...

Yes, I was thinking on how to make a test for this, as I don't see it straightforward... Since lgb.cv() basically returns the results list, I could only think about seeding everything (model through params and nfold) and check that result changes with and without train_folds.

But perhaps there is a better way I did not think of...

[jameslamb]

No problem, and no rush at all!

Let me know if it's too simplistic, but what about this?

1. Construct a dataset for a regression tasks where the relationship between the features and target should be easy for LightGBM to figure out. Like maybe where you have a few features that are all some_constant * y + some_noise()
2. Construct a second, much larger dataset with the same number of features that is all random noise
3. Set folds to only slices of the "real" data (so you aren't evaluating on any of the noise data)
4. Run lgb.cv() twice, once without train_folds and once with train_folds set to only contain indices from the "real data". Make num_iterations and num_leaves fairly small (like num_iterations = 5L, num_leaves = 5L).
5. Compare model performance between the two cases, evaluated only against the "real" data. The model trained on only the "real data" (where you specified train_folds) should have much better performance than the one trained on mostly noise.

[jameslamb]

gently pinging @julioasotodv , is there anything I can do to help you with this?

[jameslamb]

For now, I'm going to close this PR due to lack of response. @julioasotodv, thank you again for your interest in LightGBM and a great issue write-up in #3924! If you have the time and interest to contribute this in the future, we'd welcome a new pull request.

I think providing better support for customization of the cross-validation process (like for cases mentioned in #3924 where you want to do time-based splits) is a valuable contribution to the R package, but not one that will be done by maintainers before the next release (#4310).

I'm going to lock discussion on this PR for now to focus all discussion of this feature back on #3924. Anyone who is interested in contributing this feature (or wants to ask maintainers to give it higher priority and implement it) is welcome to comment on #3924.