search

NISOx-BDI / SwE-toolbox

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

SwE WB housekeeping #94

Open nicholst opened 5 years ago

nicholst commented 5 years ago

In reviewing swe_cp_WB.m I found a few passages that should be reviewed by @BryanGuillaume with help from @TomMaullin

TomMaullin commented 5 years ago

Hi @nicholst ,

 What is %#ok<AGROW>? e.g. here https://github.com/NISOx-BDI/SwE-toolbox/blob/master/swe_cp_WB.m#L940-L951

These have been here since before I worked on this code. I believe they are suppressing warnings about not pre-allocating matrix sizes. I believe we could probably remove these by pre-allocating the matrices. It will not make much difference to the code but would be better practice. I am happy to do this if you would like?

https://github.com/NISOx-BDI/SwE-toolbox/blob/master/swe_cp_WB.m#L512-L514 Why is this assignment of edof_Gr commented out?

I honestly cannot say. This was added before I worked on the code (see Bryan's last commit here)

TFCE implementation was, I thought, complete, but there are two 'TODO's here https://github.com/NISOx-BDI/SwE-toolbox/blob/master/swe_cp_WB.m#L1253 & https://github.com/NISOx-BDI/SwE-toolbox/blob/master/swe_cp_WB.m#L1275 ... compete or remove.

TFCE is implemented for nii/img input but not for mat input. These TODO's are in the .mat sections of the code. Apologies I left these here to remind myself to make an issue but never got around to asking if it was something you wanted implemented in the future. Shall I make an issue for this?

Search for all commented-out code and see if it can be now removed (e.g. https://github.com/NISOx-BDI/SwE-toolbox/blob/master/swe_cp_WB.m#L1486 )

Most of the commented out code in these sections I have never touched and been reluctant to remove in case they may be part of something that was never fully implemented that may be implemented in the future. I am happy to remove these if you are but I can't say whether these are useful or not.

 Review indenting / style. I tried to implement consistent indenting as part of #93

Yes I have been meaning to do this for some time. I will add this to your PR shortly.

nicholst commented 5 years ago

Thanks @TomMaullin - Now that we know that @BryanGuillaume is joining, he can pitch in on some of these.

And, thanks in particular for explaining %#ok<AGROW>... I should have just Googled it :)

TomMaullin commented 5 years ago

Okay great - thanks @nicholst .

nicholst commented 5 years ago

Tagging @BryanGuillaume