Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SwE WB housekeeping #94

Open
5 tasks
nicholst opened this issue Dec 21, 2018 · 4 comments
Open
5 tasks

SwE WB housekeeping #94

nicholst opened this issue Dec 21, 2018 · 4 comments

Comments

@nicholst
Copy link
Collaborator

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

@TomMaullin
Copy link
Collaborator

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
Copy link
Collaborator Author

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
Copy link
Collaborator

Okay great - thanks @nicholst .

@nicholst
Copy link
Collaborator Author

Tagging @BryanGuillaume

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants