Add shuffle parameter to KFoldCV constructor

[rcurtin]

I've modified the KFoldCV class so that there is an optional 'shuffle' parameter, and made a few other changes also:

• Better handling of sparse matrices to fix ShuffleData directly reads sparse matrix arrays #1411 throughout the codebase.

• Better handling of datasets where the number of points is not evenly divisible by k in KFoldCV. @micyril, if you like, you can take a look at what I did and see if there are any issues. Basically the modification amounts to holding a lastBinSize, since the last cross-validation bin may hold fewer points than the others, and then modifying the GetTrainingSubset() and GetValidationSubset() functions.

The shuffling is done at the beginning of any call to KFoldCV::Evaluate().

This fixes #1409.

[octogman]

Awesome Ryan, thanks for adding data shuffling!

[micyril]

If I understand it right, the whole story with lastBinSize is to make use of all data when the number of the samples is not divisible by k (the previous code will miss some samples during each run in this case). If it's true, then I guess we need to use this "last bin" in a training subset for i from 1 to k - 1 rather than just when i == k - 1. We probably also should get rid of the variable trainingSubsetSize, since with this implementation it makes the code more confusing.

[micyril]

By reading your initial message for this PR I see that we are probably not on same page about the last cross-validation bin. I think that when we do something like this

binSize = source.n_cols / k;
trainingSubsetSize = binSize * (k - 1);
lastBinSize = source.n_cols - ((k - 1) * binSize);

lastBinSize is actually no less than binSize.

[rcurtin]

Since source.n_cols / k is integer math it's effectively floor(source.n_cols / k), so actually I had thought about this a little wrong. I had been thinking lastBinSize would be less than binSize but it will actually be up to k larger than binSize. But I think that is no problem.

I agree that trainingSubsetSize is now confusing since it can change, so I've removed the variable and replaced it with the manual calculation of binSize * (k - 1) or binSize * (k - 2) + lastBinSize.

[micyril]

I'm not sure I understand the logic behind the condition i == k - 1. In my opinion it should be i != 0 since when i == 0 the last bin will be used for validation, and in all other cases it should be used for training.

[rcurtin]

You're right, thanks for the clarification.

[micyril]

Probably it's better to use the name "predictions" instead of "labels" since k-fold cross-validation can be applied to such methods as LinearRegression. Also probably it's better to use the method cols rather than subvec if one day we want to use it with something like FFN (which in the current implementation uses arma::mat for responses).

[rcurtin]

Actually I'll just change them to xsOrig and ysOrig, there is no confusion there. And you are right, cols() would be better.

[micyril]

When I was implementing KFoldCV, I also was thinking about the need to shuffle the passed data. But I was thinking about doing it once during construction of a KFoldCV object rather than before each call of Evaluate(). Is it a good practice to do it for each set of hyper-parameters? If so, can you provide some references?

[rcurtin]

Agreed, I was not thinking of the hyper-parameter tuning usage. So I changed the functionality---now if shuffle is specified to the constructor, it shuffles during object construction, and shuffling can be performed again manually with the Shuffle() function.

[micyril]

There are no more comments from my side.

[rcurtin]

@micyril thanks for the review---I'll go ahead and merge this in 3 days to leave time for any other comments.