Bugfix for 2767 - fix rf path trying to sample 0 columns

[drobison00]

In some situations, the number of sampled columns, multiplied by max features would result in a number that rounded to 0, which in turn caused a memory block of size zero to be allocated, and CudaMemsetAsync to throw an invalid argument exception.

This sets a floor of 1 for ncols_sampled, which resolves the issue.

Closes #2767

[GPUtester]

Please update the changelog in order to start CI tests.

View the gpuCI docs here.

[beckernick]

Glad to see this is so clean!

Just curious why this only came up with RF regressor, rather than both the RF classifier and regressor. Does the classifier segment have a guard against this? Seems like this is used in both downstream in the //regression and //classifcation sections.

[drobison00]

@beckernick
I tested a bit with the equivalent classification paths. They are affected, in that 'histcount', defined in memory.cuh:218, will be set to zero, and all the subsequent (h/d)_hist_xxx buffer allocations become zero, and result in 0x00 data pointers in the tree data structure. I'm not familiar enough with the code paths, but It seems unlikely that everything was working as expected with the classifier, when ncols * max_features < 1, on that path.

[beckernick]

@beckernick
I tested a bit with the equivalent classification paths. They are affected, in that 'histcount', defined in memory.cuh:218, will be set to zero, and all the subsequent (h/d)_hist_xxx buffer allocations become zero, and result in 0x00 data pointers in the tree data structure. I'm not familiar enough with the code paths, but It seems unlikely that everything was working as expected with the classifier, when ncols * max_features < 1, on that path.

Got it, makes sense. A silent failure is just as devious.

Looks like a FIL test is now failing, but it seems like that would be unrelated to this code path. It also seems to be failing in another PR as well, further suggesting that (#2789)

FAILED cuml/test/test_fil.py::test_lightgbm - assert False

cc @dantegd is this possibly an expected failure?

[dantegd]

rerun tests

[dantegd]

@beckernick PR #2787 disabled that test temporarily because there seems to be a small issue with that test and lightgbm 3.0

[dantegd]

Change lgtm

[beckernick]

Ah, perfect. thanks for the quick explanation 👍