mixOmics icon indicating copy to clipboard operation
mixOmics copied to clipboard

Fix for Issue #213

Open Max-Bladen opened this issue 2 years ago • 3 comments

As correctly identified by user @VilenneFrederique, the tune.block.splsda() function was behavioring incorrectly when using validation = "loo", such that the n parameter (denotes sample size) was only being set in the if(validation == "Mfold") block. This caused issues down stream as 1:n is called. If using LOOCV, then n was equal to NA, raising the error.

Noticed that the warning messages about using a folds value not equal to 1 in LOOCV situations wasn't printing. Adjusted it so users were notified of this as well as expanded some of the stop() calls to more specifically notify users of what the issue was with their inputted value of folds.

Max-Bladen avatar May 10 '22 23:05 Max-Bladen

Thanks @Max-Bladen for implementing this fix and refactoring the code along the way. It's always a good excuse to increase the code quality as we fix things. For any fix, I would always start by adding a unit test that fails before the fix and passes after it. I think we didn't take this approach here. Also, I noticed you removed the error for when nrepeat != 1 with validation == 'loo'. That behaviour is indeed intended so we educate users and make sure they would not mistakenly consider a leave one out validation as 'repeated' cross-validation, as we have seen in the past. Therefore, I'd restore that part.

aljabadi avatar Jun 03 '22 08:06 aljabadi

Also, this was a good case of good-first-review. Basically, any change of such small diff and such clear issue from user can be tagged as such. As a general rule, if you can break down the changes to as small a diff as possible, it would be massively easier for the reviewer. That is a should-have at the commit level, and nice to have at the PR level.

aljabadi avatar Jun 03 '22 08:06 aljabadi

Cheers @aljabadi.

Regarding the removed nrepeat != 1 error, it was removed due to redundancy. That if statement in MCV.block.splsda() will never get called as before that in tune.block.splsda(), if validation == "loo" and nepeat != 1, nrepeat is SET to 1. Refer here. I've restored it as you've asked - however I've noticed a bunch of these redundant sections of code and assumed that cleaning them up would be best.

If PR's fail due to coverage reductions, I'm only adding test cases in as a bandaid. As you know, I'm working on a large overhaul of the entire testing functionality. This is requiring me to essentially rewrite all tests for all functions so I didn't want to waste too much time on a test case that would be rewritten anyway.

To clarify your second comment, would it be preferable for me to have only addressed the bug in this PR and then had a separate PR where I refactored a bit of the code?

Let me know what suits you

Max-Bladen avatar Jun 08 '22 22:06 Max-Bladen