[FIX] align F tests specification with BIDS stats model (#1276)

Remi-Gau · web-flow · commit d02442603a2f · 2024-07-10T07:54:20.000+02:00
* fix F tests

* fix tests

* update changelog
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -41,6 +41,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Changed
 
+* [ENH] align specification of F contrasts on the BIDS stats model: they should now be specified as a 2D matrix and not a 1D vector. #1276 @Remi-Gau
 * [DOC] change theme and structure of the documentation #1256 @Remi-Gau
 * [REF] Refactor and update CLI in #1096 @Remi-Gau
 * [ENH] {func}`getData` only loads anat data when requested #1257 @Remi-Gau
@@ -55,7 +56,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 * [FIX] update {func}`createDefaultStatsModel` to use proper `GroupBy` at the dataset level #1248 by @d-ni374
 * [FIX] make {func}`getAcquisitionTime` less brittle #1248 by @d-ni374
-* [FIX] fix regular expression in {func}`bidsResults` to identify contrasts #1248 by @d-ni374
+* [FIX] fix regular expression in {func}`bidsResults` to identify contrasts #1248 by @d-ni374 and #1275 by @Remi-Gau
 * [FIX] pass analysis level to stats actions when using python CLI #1258 @Remi-Gau
 * [FIX] remove goodness of fit from dataset level analysis as it is not supported by the MACS toolbox #1265 by @Remi-Gau
 * [FIX] add java and zip to container recipes to allow using nidm results with octave #1265 by @Remi-Gau
diff --git a/demos/face_repetition/models/model-faceRepetition_smdl.json b/demos/face_repetition/models/model-faceRepetition_smdl.json
@@ -161,12 +161,54 @@
             "rot_z"
           ],
           "Weights": [
-            1,
-            1,
-            1,
-            1,
-            1,
-            1
+            [
+              1,
+              0,
+              0,
+              0,
+              0,
+              0
+            ],
+            [
+              0,
+              1,
+              0,
+              0,
+              0,
+              0
+            ],
+            [
+              0,
+              0,
+              1,
+              0,
+              0,
+              0
+            ],
+            [
+              0,
+              0,
+              0,
+              1,
+              0,
+              0
+            ],
+            [
+              0,
+              0,
+              0,
+              0,
+              1,
+              0
+            ],
+            [
+              0,
+              0,
+              0,
+              0,
+              0,
+              1
+            ]
           ],
           "Test": "F"
         }
diff --git a/lib/bids-matlab b/lib/bids-matlab
@@ -1 +1 @@
-Subproject commit f57e1fc759bc4656aa96782db81ca7dcf80457f6
+Subproject commit 845b186629f421840d5cf43e197655b597cf4dd9
diff --git a/src/bids_model/checkContrast.m b/src/bids_model/checkContrast.m
@@ -1,18 +1,19 @@
-function contrast = checkContrast(node, iCon)
+function contrast = checkContrast(model, node, iCon)
   %
   % Validates contrast specification
   %
-  % put some of that in bids.Model
   %
   % USAGE::
   %
-  %     contrast = checkContrast(node, iCon)
+  %     contrast = checkContrast(model, node, iCon)
   %
   %
 
+  % TODO put some of that in bids.Model
+
   % (C) Copyright 2022 bidspm developers
 
-  BidsModel.validate_constrasts(node);
+  model.validate_constrasts(node);
 
   if ~ismember(lower(node.Level), {'run', 'session', 'subject'}) && ...
           ~isTtest(node.Contrasts(iCon))
diff --git a/src/bids_model/getContrastsFromParentNode.m b/src/bids_model/getContrastsFromParentNode.m
@@ -22,7 +22,7 @@
   if isfield(sourceNode, 'Contrasts') && ~isempty(sourceNode.Contrasts)
     for i = 1:numel(sourceNode.Contrasts)
       if isTtest(sourceNode.Contrasts{i}) % only contrast can be forwarded
-        contrastsList{end + 1} = checkContrast(sourceNode, i);
+        contrastsList{end + 1} = checkContrast(model, sourceNode, i);
       end
     end
 
diff --git a/src/bids_model/getContrastsList.m b/src/bids_model/getContrastsList.m
@@ -31,7 +31,7 @@
   if isfield(node, 'Contrasts')
 
     for i = 1:numel(node.Contrasts)
-      contrastsList{end + 1} = checkContrast(node, i); %#ok<*AGROW>
+      contrastsList{end + 1} = checkContrast(model, node, i); %#ok<*AGROW>
     end
 
     % check previous Nodes recursively
diff --git a/src/stats/subject_level/specifyContrasts.m b/src/stats/subject_level/specifyContrasts.m
@@ -180,13 +180,13 @@
 
   for iCon = 1:length(node.Contrasts)
 
-    this_contrast = checkContrast(node, iCon);
+    thisContrast = checkContrast(model, node, iCon);
 
-    if isempty(this_contrast) || strcmp(this_contrast.Test, 'pass')
+    if isempty(thisContrast) || strcmp(thisContrast.Test, 'pass')
       continue
     end
 
-    conditionList = this_contrast.ConditionList;
+    conditionList = thisContrast.ConditionList;
 
     for iCdt = 1:length(conditionList)
       cdtName = conditionList{iCdt};
@@ -201,7 +201,7 @@
 
     if length(nbRuns) > 1
       msg = sprintf('Skipping contrast %s: runs are missing condition %s', ...
-                    this_contrast.Name, cdtName);
+                    thisContrast.Name, cdtName);
       id = 'runMissingCondition';
       logger('WARNING', msg, 'id', id, 'filename', mfilename());
 
@@ -213,22 +213,24 @@
       % Use the SPM Sess index for the contrast name
       iSess = getSessionForRegressorNb(regIdx{1}(iRun), model.SPM);
 
-      contrastName = constructContrastNameFromBidsEntity(this_contrast.Name, model.SPM, iSess);
-      C = newContrast(model.SPM, contrastName, this_contrast.Test, conditionList);
+      contrastName = constructContrastNameFromBidsEntity(thisContrast.Name, model.SPM, iSess);
+      C = newContrast(model.SPM, contrastName, thisContrast.Test, conditionList);
 
       for iCdt = 1:length(conditionList)
 
-        if strcmp(this_contrast.Test, 't')
-          C.C(end, regIdx{iCdt}(iRun)) = this_contrast.Weights(iCdt);
+        if strcmp(thisContrast.Test, 't')
+          C.C(end, regIdx{iCdt}(iRun)) = thisContrast.Weights(iCdt);
 
-        elseif strcmp(this_contrast.Test, 'F')
-          C.C(iCdt, regIdx{iCdt}(iRun)) = this_contrast.Weights(iCdt);
+        elseif strcmp(thisContrast.Test, 'F')
+          for i = 1:size(thisContrast.Weights, 1)
+            C.C(i, regIdx{iCdt}(iRun)) = thisContrast.Weights(i, iCdt);
+          end
 
         end
 
       end
 
-      [contrasts, counter] = appendContrast(contrasts, C, counter, this_contrast.Test);
+      [contrasts, counter] = appendContrast(contrasts, C, counter, thisContrast.Test);
 
     end
 
diff --git a/src/stats/subject_level/specifySessionLvlContrasts.m b/src/stats/subject_level/specifySessionLvlContrasts.m
@@ -32,7 +32,7 @@
   % amongst themselves or inferior to baseline
   for iCon = 1:length(node.Contrasts)
 
-    this_contrast = checkContrast(node, iCon);
+    this_contrast = checkContrast(model, node, iCon);
 
     if isempty(this_contrast) || strcmp(this_contrast.Test, 'pass')
       continue
diff --git a/src/stats/subject_level/specifySubLvlContrasts.m b/src/stats/subject_level/specifySubLvlContrasts.m
@@ -36,7 +36,7 @@
   % amongst themselves or inferior to baseline
   for iCon = 1:length(node.Contrasts)
 
-    thisContrast = checkContrast(node, iCon);
+    thisContrast = checkContrast(model, node, iCon);
 
     if isempty(thisContrast) || strcmp(thisContrast.Test, 'pass')
       continue
@@ -123,9 +123,12 @@
       C.C(end, regIdx) = thisContrast.Weights(iCdt);
 
     elseif strcmp(thisContrast.Test, 'F')
+
       for i = 1:numel(regIdx)
-        C.C(row, regIdx(i)) = thisContrast.Weights(iCdt);
-        row = row + 1;
+        for i_w = 1:size(thisContrast.Weights, 1)
+          C.C(row, regIdx(i)) = thisContrast.Weights(i_w, iCdt);
+          row = row + 1;
+        end
       end
 
     end
@@ -134,6 +137,9 @@
 
   end
 
+  rows_to_rm = all(C.C == 0, 2);
+  C.C(rows_to_rm, :) = [];
+
   % do not create this contrast if a condition is missing
   if exist('status', 'var')
     if ~status
diff --git a/tests/data/models/model-vismotionFtest_smdl.json b/tests/data/models/model-vismotionFtest_smdl.json
@@ -70,8 +70,14 @@
             "VisStat"
           ],
           "Weights": [
-            1,
-            1
+            [
+              1,
+              0
+            ],
+            [
+              0,
+              1
+            ]
           ],
           "Test": "F"
         }
diff --git a/tests/tests_QA/test_computeDesignEfficiency.m b/tests/tests_QA/test_computeDesignEfficiency.m
@@ -51,7 +51,7 @@ function test_computeDesignEfficiency_vislocalizer()
   json.Nodes{runNodeIdx}.Model.X = {'trial_type.cdt_A', 'trial_type.cdt_B'};
   json.Nodes{runNodeIdx}.DummyContrasts.Contrasts = {'trial_type.cdt_A', 'trial_type.cdt_B'};
 
-  contrast = struct('Type', 't', ...
+  contrast = struct('Test', 't', ...
                     'Name', 'A_gt_B', ...
                     'Weights', [1, -1], ...
                     'ConditionList', {{'trial_type.cdt_A', 'trial_type.cdt_B'}});
diff --git a/tests/tests_bids_model/test_getContrastsList.m b/tests/tests_bids_model/test_getContrastsList.m
@@ -36,6 +36,7 @@ function test_getContrastsList_basic()
 
   opt = setOptions('vismotion', {'01' 'ctrl01'}, 'pipelineType', 'stats');
 
+  opt.model.bm.Nodes{3}.Contrasts{1}.Test = 't';
   opt.model.bm.Nodes{3}.Contrasts{1}.Name = 'foo';
   opt.model.bm.Nodes{3}.Contrasts{1}.ConditionList = {'foo'};
   opt.model.bm.Nodes{3}.Contrasts{1}.Weights = -1;
diff --git a/tests/tests_stats/subject_level/test_specifyContrasts.m b/tests/tests_stats/subject_level/test_specifyContrasts.m
@@ -8,105 +8,6 @@
   initTestSuite;
 end
 
-function test_specifyContrasts_vismotion_F_contrast()
-  %
-  % Note requires an SPM.mat to run
-  %
-
-  % GIVEN
-  subLabel = '01';
-
-  opt = setOptions('vismotion', subLabel, 'pipelineType', 'stats');
-
-  ffxDir = getFFXdir(subLabel, opt);
-  spmMatFile = cellstr(fullfile(ffxDir, 'SPM.mat'));
-  load(spmMatFile{1}, 'SPM');
-
-  opt.model.file = spm_file(opt.model.file, 'basename', 'model-vismotionFtest_smdl');
-  model = BidsModel('file', opt.model.file);
-
-  % WHEN
-  contrasts = specifyContrasts(model, SPM);
-
-  % THEN
-  expected(1).name = 'VisMot_gt_VisStat_ses-01'; %#ok<*AGROW>
-  expected(1).C = [1 -1 0 0 0 0 0 0 0];
-  expected(1).type = 't';
-
-  expected(2).name = 'F_test_mot_static_ses-01'; %#ok<*AGROW>
-  expected(2).C = [1 0 0 0 0 0 0 0 0
-                   0 1 0 0 0 0 0 0 0];
-  expected(2).type = 'F';
-
-  assertEqual(contrasts, expected);
-
-end
-
-function test_specifyContrasts_subject_level_F_contrast()
-  %
-  % to test the generation of F contrasts when there are several runs
-  %
-
-  taskName = 'motion';
-
-  model = BidsModel('init', true);
-
-  model.Input.task = taskName;
-
-  model.Nodes{1, 1}.Model.X = {'motion', 'static'};
-  model.Nodes{end}.Level = 'Run';
-  model.Nodes{end}.Name = 'run_level';
-  model.Nodes{end}.GroupBy = {'run', 'subject'};
-  model.Nodes{end} = rmfield(model.Nodes{1}, 'Contrasts');
-  model.Nodes{end} = rmfield(model.Nodes{1}, 'DummyContrasts');
-
-  model.Nodes{2, 1}.Model.X = 1;
-  model.Nodes{end}.Level = 'Subject';
-  model.Nodes{end}.Name = 'subject_level';
-  model.Nodes{end}.GroupBy = {'contrast', 'subject'};
-  model.Nodes{end}.Contrasts{1} = struct('Test', 'F', ...
-                                         'Name', 'F_test_mot_static', ...
-                                         'ConditionList', {{'motion', 'static'}}, ...
-                                         'Weights', [1 1]);
-
-  SPM.Sess(1).col = [1, 2, 3];
-  % skip Sess 2 to make sure contrast naming is based on the Sess number
-  SPM.Sess(3).col = [4, 5, 6];
-  SPM.Sess(4).col = [7, 8, 9];
-  SPM.xX.name = { ...
-                 ' motion*bf(1)'
-                 ' static*bf(1)'
-                 ' rot_x'
-                 ' motion*bf(1)'
-                 ' static*bf(1)'
-                 ' rot_x'
-                 ' motion*bf(1)'
-                 ' static*bf(1)'
-                 ' rot_x'
-                };
-
-  SPM.xX.X = ones(1, numel(SPM.xX.name));
-
-  % WHEN
-  contrasts = specifyContrasts(model, SPM);
-
-  % THEN
-  expected.name =  'F_test_mot_static';
-
-  expected.C = zeros(6, 9);
-  expected.C(1, 1) = 1;
-  expected.C(2, 4) = 1;
-  expected.C(3, 7) = 1;
-  expected.C(4, 2) = 1;
-  expected.C(5, 5) = 1;
-  expected.C(6, 8) = 1;
-
-  expected.type = 'F';
-
-  assertEqual(contrasts, expected);
-
-end
-
 function test_specifyContrasts_bug_854()
 
   % GIVEN
diff --git a/tests/tests_stats/subject_level/test_specifyContrasts_F.m b/tests/tests_stats/subject_level/test_specifyContrasts_F.m

Original file line number	Diff line number	Diff line change
`@@ -1,18 +1,19 @@`
`1`		`-function contrast = checkContrast(node, iCon)`
	`1`	`+function contrast = checkContrast(model, node, iCon)`
`2`	`2`	`%`
`3`	`3`	`% Validates contrast specification`
`4`	`4`	`%`
`5`		`- % put some of that in bids.Model`
`6`	`5`	`%`
`7`	`6`	`% USAGE::`
`8`	`7`	`%`
`9`		`- % contrast = checkContrast(node, iCon)`
	`8`	`+ % contrast = checkContrast(model, node, iCon)`
`10`	`9`	`%`
`11`	`10`	`%`
`12`	`11`
	`12`	`+ % TODO put some of that in bids.Model`
	`13`	`+`
`13`	`14`	`% (C) Copyright 2022 bidspm developers`
`14`	`15`
`15`		`- BidsModel.validate_constrasts(node);`
	`16`	`+ model.validate_constrasts(node);`
`16`	`17`
`17`	`18`	`if ~ismember(lower(node.Level), {'run', 'session', 'subject'}) && ...`
`18`	`19`	`~isTtest(node.Contrasts(iCon))`