SnPM-toolbox / SnPM-devel

Statistical NonParametric Mapping, development version
GNU General Public License v3.0
25 stars 10 forks source link

Release SnPM 13.1.08 #70

Closed cmaumet closed 6 years ago

cmaumet commented 6 years ago

This PR includes updates for the next release of SnPM:

cmaumet commented 6 years ago

Hi @nicholst,

Tests currently report 1 failure:

Failure Summary:

     Name                                                              Failed  Incomplete  Reason(s)
    =================================================================================================
     test_multisub_withinsubanova/test_multisub_withinsubanova_approx    X         X       Errored.

Totals:
   69 Passed, 1 Failed, 1 Incomplete.
   525.0753 seconds testing time.

It looks like changes in https://github.com/SnPM-toolbox/SnPM-devel/pull/61, in particular https://github.com/SnPM-toolbox/SnPM-devel/pull/61/commits/e38507307c12ee29cbfe98eed9a498c44b463b77, are breaking the tests. Complete log:

>> run(test_multisub_withinsubanova, 'test_multisub_withinsubanova_approx')
Running test_multisub_withinsubanova

------------------------------------------------------------------------
Running job #6
------------------------------------------------------------------------
Running 'Within Subject ANOVA, k diffs/contrasts per subject'
Failed  'Within Subject ANOVA, k diffs/contrasts per subject'
Error using snpm_pi_ANOVAwithinS (line 280)
Bad PiCond ()
In file "/Users/camaumet/Softs/SnPM-devel/spm12/toolbox/SnPM13/snpm_pi_ANOVAwithinS.m" (???), function "snpm_pi_ANOVAwithinS" at line 280.
In file "/Users/camaumet/Softs/SnPM-devel/spm12/toolbox/SnPM13/snpm_ui.m" (???), function "snpm_ui" at line 297.
In file "/Users/camaumet/Softs/SnPM-devel/spm12/toolbox/SnPM13/config/snpm_run_ui.m" (???), function "snpm_run_ui" at line 12.

No executable modules, but still unresolved dependencies or incomplete module inputs.
The following modules did not run:
Failed: Within Subject ANOVA, k diffs/contrasts per subject
Skipped: Compute
Skipped: Inference

@nicholst: can you have a look to propose a fix? I would change ~(nSubj<=12 || ~bAproxTst) into (nSubj<=12 || ~bAproxTst) but not sure whether that would break the case you were trying to fix in #61.

nicholst commented 6 years ago

Ug... this is annoying. I don't think the fix you suggest works. The setup is that we have 'exact' and 'random' methods...

if nSubj<=12 || ~bAproxTst                    % exact method
 [...]    
else                                          % random method
 [...]
end

and then

elseif length(perm)==0 && ~(nSubj<=12 || ~bAproxTst)
    % Special case where random method missed iCond; order of perms is
    % random so can we can just replace first perm.
    PiCond(1,:) = iCond;
    perm = 1;

So the logic seems right... if the correctly labeled permutation isn't found AND NOT(exact) then you can add it to the top; but if the correctly labeled permutation isn't found AND Exact there's a problem.

Can you direct me to the test case that generates this? I'll have a look... though realistically not before Thursday.

cmaumet commented 6 years ago

Right! I always get these wrong...

Anyway, the failing test case is test_multisub_withinsubanova_approx:

Let me know!

nicholst commented 6 years ago

OK @cmaumet! Hopefully https://github.com/cmaumet/SnPM-devel/pull/1 will fix this... hallelujah for testing!

cmaumet commented 6 years ago

Thanks @nicholst! Your update fixed the issue but then the tests were failing because the fix also introduced a change in the selected permutations for approximate analyses.

This is a problem for the non-regression tests were we check that the produced outputs are identical to the ground truth outputs. We could update SnPM8 and recompute the ground truth to match what you have done in https://github.com/SnPM-toolbox/SnPM-devel/pull/70/commits/264fb2c7ea1bea5d1c3b5e2668ae87d558c48d6b. Another solution is to change slightly your update to preserve the order we had before for the permutations. That's what I've done in 12872ece4c0f4b2594ee5a958a29e57b4a549fa6. Let me know if that's fine with you.

All tests now pass:

Totals:
   70 Passed, 0 Failed, 0 Incomplete.
   179.9142 seconds testing time.
nicholst commented 6 years ago

I thought of this belatedly.

Yes, I'm fine with this. Thank you!