[MAINT] refactor of tests and code in GLM module (#4066)

* refactor tests contrasts

* refactor tests glm

* refactor tests glm

* refactor tests glm

* refactor tests glm

* refactor tests glm

* refactor tests glm

* refactor tests glm

* refactor tests glm

* deal with back funkyness

* test error messages

* rm extraspace

* enforce encoding

* revert

* remove test and exception

* fix tests

* fix typo

nilearn/glm/_utils.py

@@ -67,9 +67,8 @@ def _check_and_load_tables(tables_, var_name):
             pass
         else:
             raise TypeError(
-                "%s can only be a pandas DataFrames or a"
-                "string. A %s was provided at idx %d"
-                % (var_name, type(table), table_idx)
+                f"{var_name} can only be a pandas DataFrames or a string. "
+                f"A {type(table)} was provided at idx {table_idx}"
             )
     return tables
 
@@ -109,6 +108,8 @@ def _check_events_file_uses_tab_separators(events_files):
     if not isinstance(events_files, (list, tuple)):
         events_files = [events_files]
     for events_file_ in events_files:
+        if isinstance(events_file_, (pd.DataFrame)):
+            continue
         try:
             with open(events_file_) as events_file_obj:
                 events_file_sample = events_file_obj.readline()
@@ -118,12 +119,6 @@ def _check_events_file_uses_tab_separators(events_files):
             Handling them here will beak the calling code,
             and refactoring that is not straightforward.
             """
-        except TypeError:  # events is Pandas dataframe.
-            pass
-        except UnicodeDecodeError:  # py3:if binary file
-            raise ValueError(
-                "The file does not seem to be a valid unicode text file."
-            )
         except OSError:  # if invalid filepath.
             pass
         else:
@@ -132,13 +127,13 @@ def _check_events_file_uses_tab_separators(events_files):
                     sample=events_file_sample,
                     delimiters=valid_separators,
                 )
-            except csv.Error:
+            except csv.Error as e:
                 raise ValueError(
                     "The values in the events file "
                     "are not separated by tabs; "
                     "please enforce BIDS conventions",
                     events_file_,
-                )
+                ) from e
 
 
 def _check_run_tables(run_imgs, tables_, tables_name):

nilearn/glm/contrasts.py

@@ -145,16 +145,13 @@ def _compute_fixed_effect_contrast(
     n_contrasts = 0
     for i, (lab, res, con_val) in enumerate(zip(labels, results, con_vals)):
         if np.all(con_val == 0):
-            warn(f"Contrast for session {int(i)} is null")
+            warn(f"Contrast for session {int(i)} is null.")
             continue
         contrast_ = compute_contrast(lab, res, con_val, contrast_type)
-        if contrast is None:
-            contrast = contrast_
-        else:
-            contrast = contrast + contrast_
+        contrast = contrast_ if contrast is None else contrast + contrast_
         n_contrasts += 1
     if contrast is None:
-        raise ValueError("all contrasts provided were null contrasts")
+        raise ValueError("All contrasts provided were null contrasts.")
     return contrast * (1.0 / n_contrasts)
 
 
@@ -216,10 +213,7 @@ def __init__(
         self.effect = effect
         self.variance = variance
         self.dof = float(dof)
-        if dim is None:
-            self.dim = effect.shape[0]
-        else:
-            self.dim = dim
+        self.dim = effect.shape[0] if dim is None else dim
         if self.dim > 1 and contrast_type == "t":
             print("Automatically converted multi-dimensional t to F contrast")
             contrast_type = "F"
@@ -297,7 +291,7 @@ def p_value(self, baseline=0.0):
             p-values, one per voxel
 
         """
-        if self.stat_ is None or not self.baseline == baseline:
+        if self.stat_ is None or self.baseline != baseline:
             self.stat_ = self.stat(baseline)
         # Valid conjunction as in Nichols et al, Neuroimage 25, 2005.
         if self.contrast_type == "t":
@@ -329,7 +323,7 @@ def one_minus_pvalue(self, baseline=0.0):
             one_minus_pvalues, one per voxel
 
         """
-        if self.stat_ is None or not self.baseline == baseline:
+        if self.stat_ is None or self.baseline != baseline:
             self.stat_ = self.stat(baseline)
         # Valid conjunction as in Nichols et al, Neuroimage 25, 2005.
         if self.contrast_type == "t":
@@ -360,7 +354,7 @@ def z_score(self, baseline=0.0):
             statistical values, one per voxel
 
         """
-        if self.p_value_ is None or not self.baseline == baseline:
+        if self.p_value_ is None or self.baseline != baseline:
             self.p_value_ = self.p_value(baseline)
         if self.one_minus_pvalue_ is None:
             self.one_minus_pvalue_ = self.one_minus_pvalue(baseline)
@@ -473,7 +467,7 @@ def compute_fixed_effects(
     if n_runs != len(variance_imgs):
         raise ValueError(
             f"The number of contrast images ({len(contrast_imgs)}) differs "
-            f"from the number of variance images ({len(variance_imgs)}). "
+            f"from the number of variance images ({len(variance_imgs)})."
         )
 
     if isinstance(mask, NiftiMasker):

nilearn/glm/first_level/design_matrix.py

@@ -442,7 +442,7 @@ def check_design_matrix(design_matrix):
         Per-event onset time (in seconds)
 
     """
-    names = [name for name in design_matrix.keys()]
+    names = list(design_matrix.keys())
     frame_times = design_matrix.index
     matrix = design_matrix.values
     return frame_times, matrix, names

nilearn/glm/first_level/first_level.py

@@ -168,7 +168,7 @@ def run_glm(
     acceptable_noise_models = ["ols", "arN"]
     if (noise_model[:2] != "ar") and (noise_model != "ols"):
         raise ValueError(
-            f"Acceptable noise models are {acceptable_noise_models}."
+            f"Acceptable noise models are {acceptable_noise_models}. "
             f"You provided 'noise_model={noise_model}'."
         )
     if Y.shape[0] != X.shape[0]:
@@ -439,10 +439,7 @@ def __init__(
         self.target_shape = target_shape
         self.smoothing_fwhm = smoothing_fwhm
         memory = stringify_path(memory)
-        if isinstance(memory, str):
-            self.memory = Memory(memory)
-        else:
-            self.memory = memory
+        self.memory = Memory(memory) if isinstance(memory, str) else memory
         self.memory_level = memory_level
         self.standardize = standardize
         if signal_scaling is False:
@@ -652,7 +649,7 @@ def fit(
                     confounds_matrix = confounds[run_idx].values
                     if confounds_matrix.shape[0] != n_scans:
                         raise ValueError(
-                            "Rows in confounds does not match"
+                            "Rows in confounds does not match "
                             "n_scans in run_img "
                             f"at index {run_idx}."
                         )
@@ -779,7 +776,7 @@ def compute_contrast(
 
         """
         if self.labels_ is None or self.results_ is None:
-            raise ValueError("The model has not been fit yet")
+            raise ValueError("The model has not been fit yet.")
 
         if isinstance(contrast_def, (np.ndarray, str)):
             con_vals = [contrast_def]
@@ -788,13 +785,13 @@ def compute_contrast(
         else:
             raise ValueError(
                 "contrast_def must be an array or str or list of"
-                " (array or str)"
+                " (array or str)."
             )
 
         n_runs = len(self.labels_)
         n_contrasts = len(con_vals)
         if n_contrasts == 1 and n_runs > 1:
-            warn(f"One contrast given, assuming it for all {int(n_runs)} runs")
+            warn(f"One contrast given, assuming it for all {n_runs} runs")
             con_vals = con_vals * n_runs
         elif n_contrasts != n_runs:
             raise ValueError(
@@ -884,7 +881,7 @@ def _get_voxelwise_model_attribute(self, attribute, result_as_time_series):
             )
 
         if self.labels_ is None or self.results_ is None:
-            raise ValueError("The model has not been fit yet")
+            raise ValueError("The model has not been fit yet.")
 
         output = []
 

nilearn/glm/first_level/hemodynamic_models.py

@@ -542,14 +542,12 @@ def _regressor_names(con_name, hrf_model, fir_delays=None):
         names = [con_name, f"{con_name}_derivative", f"{con_name}_dispersion"]
     elif hrf_model == "fir":
         names = [f"{con_name}_delay_{int(i)}" for i in fir_delays]
-    # Handle callables
     elif callable(hrf_model):
         names = [f"{con_name}_{hrf_model.__name__}"]
     elif isinstance(hrf_model, Iterable) and all(
-        [callable(_) for _ in hrf_model]
+        callable(_) for _ in hrf_model
     ):
         names = [f"{con_name}_{model.__name__}" for model in hrf_model]
-    # Handle some default cases
     elif isinstance(hrf_model, Iterable) and not isinstance(hrf_model, str):
         names = [f"{con_name}_{i}" for i in range(len(hrf_model))]
 
@@ -640,7 +638,7 @@ def _hrf_kernel(hrf_model, tr, oversampling=50, fir_delays=None):
         except TypeError:
             raise ValueError(error_msg)
     elif isinstance(hrf_model, Iterable) and all(
-        [callable(_) for _ in hrf_model]
+        callable(_) for _ in hrf_model
     ):
         try:
             hkernel = [model(tr, oversampling) for model in hrf_model]

nilearn/glm/model.py

@@ -194,8 +194,12 @@ def Tcontrast(self, matrix, store=("t", "effect", "sd"), dispersion=None):
         # 1D vectors assumed to be row vector
         if matrix.ndim == 1:
             matrix = matrix[None]
+        if matrix.size == 0:
+            raise ValueError("t contrasts cannot be empty: " f"got {matrix}")
         if matrix.shape[0] != 1:
-            raise ValueError("t contrasts should have only one row")
+            raise ValueError(
+                "t contrasts should have only one row: " f"got {matrix}."
+            )
         matrix = pad_contrast(
             con_val=matrix, theta=self.theta, contrast_type="t"
         )
@@ -263,8 +267,8 @@ def Fcontrast(self, matrix, dispersion=None, invcov=None):
             matrix = matrix[None]
         if matrix.shape[1] != self.theta.shape[0]:
             raise ValueError(
-                f"F contrasts should have shape[1] P={self.theta.shape[0]}, "
-                f"but this has shape[1] {matrix.shape[1]}"
+                f"F contrasts should have shape[1]={self.theta.shape[0]}, "
+                f"but this has shape[1]={matrix.shape[1]}"
             )
         matrix = pad_contrast(
             con_val=matrix, theta=self.theta, contrast_type="F"

nilearn/glm/second_level/second_level.py

@@ -224,7 +224,7 @@ def _check_confounds(confounds):
             )
         # Make sure subject_label contain strings
         if not all(
-            [isinstance(_, str) for _ in confounds["subject_label"].tolist()]
+            isinstance(_, str) for _ in confounds["subject_label"].tolist()
         ):
             raise ValueError("subject_label column must contain only strings")
 
@@ -250,9 +250,10 @@ def _check_output_type(output_type, valid_types):
 
 def _check_design_matrix(design_matrix):
     """Check design_matrix type."""
-    if design_matrix is not None:
-        if not isinstance(design_matrix, pd.DataFrame):
-            raise ValueError("design matrix must be a pandas DataFrame")
+    if design_matrix is not None and not isinstance(
+        design_matrix, pd.DataFrame
+    ):
+        raise ValueError("design matrix must be a pandas DataFrame")
 
 
 def _check_effect_maps(effect_maps, design_matrix):
@@ -442,10 +443,7 @@ def __init__(
         self.target_shape = target_shape
         self.smoothing_fwhm = smoothing_fwhm
         memory = stringify_path(memory)
-        if isinstance(memory, str):
-            self.memory = Memory(memory)
-        else:
-            self.memory = memory
+        self.memory = Memory(memory) if isinstance(memory, str) else memory
         self.memory_level = memory_level
         self.verbose = verbose
         self.n_jobs = n_jobs
@@ -592,7 +590,7 @@ def compute_contrast(
 
         """
         if self.second_level_input_ is None:
-            raise ValueError("The model has not been fit yet")
+            raise ValueError("The model has not been fit yet.")
 
         # check first_level_contrast
         _check_first_level_contrast(
@@ -955,10 +953,12 @@ def non_parametric_inference(
 
     else:
         masker = clone(mask)
-        if smoothing_fwhm is not None:
-            if getattr(masker, "smoothing_fwhm") is not None:
-                warn("Parameter smoothing_fwhm of the masker overridden")
-                setattr(masker, "smoothing_fwhm", smoothing_fwhm)
+        if (
+            smoothing_fwhm is not None
+            and getattr(masker, "smoothing_fwhm") is not None
+        ):
+            warn("Parameter smoothing_fwhm of the masker overridden")
+            setattr(masker, "smoothing_fwhm", smoothing_fwhm)
 
     masker.fit(sample_map)
 

nilearn/glm/tests/test_check_events_file_uses_tab_separators.py

@@ -30,7 +30,10 @@ def _create_test_file(temp_csv, test_data, delimiter):
 
 def _run_test_for_invalid_separator(filepath, delimiter_name):
     if delimiter_name not in ("tab", "comma"):
-        with pytest.raises(ValueError):
+        with pytest.raises(
+            ValueError,
+            match="The values in the events file are not separated by tabs",
+        ):
             _check_events_file_uses_tab_separators(events_files=filepath)
     else:
         result = _check_events_file_uses_tab_separators(events_files=filepath)
@@ -86,22 +89,13 @@ def test_for_pandas_dataframe():
     assert result is None
 
 
-def test_binary_opening_an_image_error(tmp_path):
-    img_data = bytearray(
-        b"GIF87a\x01\x00\x01\x00\xe7*\x00\x00\x00\x00\x01\x01\x01\x02\x02"
-        b"\x07\x08\x08\x08\x0b\x0b\x0b\x0c\x0c\x0c\r;"
-    )
-    temp_img_file = tmp_path / "temp_img.gif"
-    with open(temp_img_file, "wb") as temp_img_obj:
-        temp_img_obj.write(img_data)
-    with pytest.raises(ValueError):
-        _check_events_file_uses_tab_separators(events_files=temp_img_file)
-
-
 def test_binary_bytearray_of_ints_data_error(tmp_path):
     temp_data_bytearray_from_ints = bytearray([0, 1, 0, 11, 10])
     temp_bin_file = tmp_path / "temp_bin.bin"
     with open(temp_bin_file, "wb") as temp_bin_obj:
         temp_bin_obj.write(temp_data_bytearray_from_ints)
-    with pytest.raises(ValueError):
+    with pytest.raises(
+        ValueError,
+        match="The values in the events file are not separated by tabs",
+    ):
         _check_events_file_uses_tab_separators(events_files=temp_bin_file)