NISOx-BDI / SwE-toolbox

SwE toolbox
GNU General Public License v2.0
16 stars 7 forks source link

Bug due to the fact that variable cmask in swe_cp is a column vector for GIfTI and CIfTI #176

Closed BryanGuillaume closed 4 years ago

BryanGuillaume commented 4 years ago

While reviewing PR #175 , I have noted that the variable cmask is a row vector for NIfTI, but a column vector for GIfTI and CIfTI. The latter can cause some unwanted effects in some places in the code. For example, the piece of code https://github.com/NISOx-BDI/SwE-toolbox/blob/5f84bdc1025dc452c7a3a943d2f8107d23713cfa/swe_cp.m#L696-L700 works only if cmask is a row vector. Indeed, if it is a column vector, the variable diffVis becomes a matrix after the loop due to the | operation that is occurring between a row and a column vector inside the loop.

As a side note, the problematic piece of code above is not always visited when doing an analysis and was not visited by the tests done so far for GIfTI and CIfTI. This explains why the bug has not been detected before now.

It seems that forcing cmask to be a row vector like for NIfTI analyses would prevent the issue noted above. But it seems important to check if this change would have some other unwanted side effects in some other parts of the code.

nicholst commented 4 years ago

OK, thanks @BryanGuillaume! Not sure of the implications of transposingcmask right now...

BryanGuillaume commented 4 years ago

This issue was solved in PR #177