search

mixOmicsTeam / mixOmics

Development repository for the Bioconductor package 'mixOmics '
http://mixomics.org/
153 stars 51 forks source link

Fix for Issue #213 #215

Closed Max-Bladen closed 1 year ago

Max-Bladen commented 2 years ago

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.

aljabadi commented 2 years ago

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 commented 2 years ago

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.

Max-Bladen commented 2 years ago

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