NISOx-BDI / SwE-toolbox

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

Crash on WB w/o cluster inference #77

Closed nicholst closed 5 years ago

nicholst commented 5 years ago

I have just found a setting that seems to crash:

and when I get to doing 'Results' I get this error:

Running 'Results Display'
Failed  'Results Display'
Error using spm_vol>spm_vol_hdr (line 80)
File "swe_clustere_Fstat_lpFWE-WB_c01.nii" does not exist.
In file "/Users/nichols/spm/spm12/spm_vol.m" (v5958), function "spm_vol_hdr" at line 80.
In file "/Users/nichols/spm/spm12/spm_vol.m" (v5958), function "spm_vol" at line 61.
In file "/Users/nichols/spm/SwE-toolbox/swe_contrasts_WB.m" (???), function "swe_contrasts_WB" at line 70.
In file "/Users/nichols/spm/SwE-toolbox/swe_getSPM.m" (???), function "swe_getSPM" at line 307.
In file "/Users/nichols/spm/SwE-toolbox/swe_results_ui.m" (v3928), function "swe_results_ui" at line 0.
In file "/Users/nichols/spm/SwE-toolbox/swe_run_results.m" (???), function "swe_run_results" at line 21.

The following modules did not run:
Failed: Results Display

Relevant line is File "swe_clustere_Fstat_lpFWE-WB_c01.nii" does not exist which is clearly an error since I didn't ask for cluster inference.

Version is '1.2.11', but more precisely, I'm working off current master. Thankful for any thoughts you have on this @TomMaullin

TomMaullin commented 5 years ago

Hi @nicholst ,

Yes, sorry! This is the use case I mentioned in our last meeting which I have fixed in the TFCE update but not in the master branch. I would have made a fix for this separately but when I made the TFCE branch I found that I was having to edit many of the same lines of code for TFCE and it would have been much more difficult to make these changes separately and then merge them later. I didn't realise at the time that TFCE would be a while in the works, else I would have fixed this error separately.

We actually did also discuss this use case a while ago when I first started working on the toolbox and decide to make it a low priority, which is why I had not fixed it sooner. I think for as long I have worked on this toolbox, until the TFCE update, we have never been able to run this use case due to one error or another.

nicholst commented 5 years ago

So sorry I’d forgotten about this. Glad it’s in the TFCE branch.

nicholst commented 5 years ago

So... close this with a reference #48 (which should be merged soon)?

TomMaullin commented 5 years ago

I could close it once #48 is merged? This way if any users find it in the meantime and go to report it, we can direct them to this thread.

nicholst commented 5 years ago

OK!

nicholst commented 5 years ago

So.... just to be clear... When clusterwise inference is selected, it cannot give an option for a threshold... rather, to be very clear, it would be good to spit out a null GUI action (did we talk about this before, where the answer is filled in and the user has no option to fill it in), like

str='Cluster forming threshold';
uth=0.001;
spm_input(str,1,'b',num2str(uth),1);

... to indicate the already-chosen cluster forming threshold.

TomMaullin commented 5 years ago

Hi @nicholst ,

Apologies I thought this was how it is implemented currently... have you found a use case in which this is not the case?

nicholst commented 5 years ago

Sorry! That was on the tfce branch. Let me try again on master. --


Thomas Nichols, PhD Professor of Neuroimaging Statistics Nuffield Department of Population Health | University of Oxford Big Data Institute | Li Ka Shing Centre for Health Information and Discovery Old Road Campus | Headington | Oxford | OX3 7LF | United Kingdom T: +44 1865 743590 | E: thomas.nichols@bdi.ox.ac.uk W: http://nisox.org | http://www.bdi.ox.ac.uk

TomMaullin commented 5 years ago

If I remember correctly this should also be implemented on the TFCE branch - did you find a use case where it was not?

nicholst commented 5 years ago

Doh! You're right... works just like I described. (On master)

TomMaullin commented 5 years ago

Ah phew! Great!

TomMaullin commented 5 years ago

This issue has now been addressed by PR #48