[MAINT] refactor of tests and code in GLM module

[Remi-Gau]

• Relates to simplify "complex" tests #3570

Changes proposed in this pull request:

• refactor and simplify complex tests in nilearn
• apply some sourcery suggested refactor in nilearn

[github-actions]

👋 @Remi-Gau Thanks for creating a PR!

Until this PR is ready for review, you can include the [WIP] tag in its title, or leave it as a github draft.

Please make sure it is compliant with our contributing guidelines. In particular, be sure it checks the boxes listed below.

• PR has an interpretable title.
• PR links to Github issue with mention Closes #XXXX (see our documentation on PR structure)
• Code is PEP8-compliant (see our documentation on coding style)
• Changelog or what's new entry in doc/changes/latest.rst (see our documentation on PR structure)

For new features:

• There is at least one unit test per new function / class (see our documentation on testing)
• The new feature is demoed in at least one relevant example.

For bug fixes:

• There is at least one test that would fail under the original bug conditions.

We will review it as quick as possible, feel free to ping us with questions if needed.

[codecov]

Codecov Report

Merging #4066 (b69f8ed) into main (0260648) will increase coverage by 0.00%.
The diff coverage is 92.00%.

@@           Coverage Diff           @@
##             main    #4066   +/-   ##
=======================================
  Coverage   91.60%   91.60%           
=======================================
  Files         143      143           
  Lines       16096    16086   -10     
  Branches     3347     3340    -7     
=======================================
- Hits        14745    14736    -9     
  Misses        804      804           
+ Partials      547      546    -1     

Flag	Coverage Δ
macos-latest_3.10	91.50% <92.00%> (+<0.01%)	⬆️
macos-latest_3.11	91.50% <92.00%> (+<0.01%)	⬆️
macos-latest_3.12	91.50% <92.00%> (+<0.01%)	⬆️
macos-latest_3.8	91.46% <92.00%> (+<0.01%)	⬆️
macos-latest_3.9	91.46% <92.00%> (+<0.01%)	⬆️
ubuntu-latest_3.10	91.50% <92.00%> (+<0.01%)	⬆️
ubuntu-latest_3.11	91.50% <92.00%> (+<0.01%)	⬆️
ubuntu-latest_3.12	91.50% <92.00%> (+<0.01%)	⬆️
ubuntu-latest_3.8	91.46% <92.00%> (+<0.01%)	⬆️
ubuntu-latest_3.9	91.46% <92.00%> (+<0.01%)	⬆️
windows-latest_3.10	91.45% <92.00%> (+0.01%)	⬆️
windows-latest_3.11	91.45% <92.00%> (+0.01%)	⬆️
windows-latest_3.12	91.45% <92.00%> (+0.01%)	⬆️
windows-latest_3.8	91.41% <92.00%> (?)
windows-latest_3.9	91.42% <92.00%> (+0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
nilearn/glm/_utils.py	90.22% <100.00%> (-0.15%)	⬇️
nilearn/glm/first_level/design_matrix.py	93.54% <100.00%> (ø)
nilearn/glm/first_level/first_level.py	93.90% <100.00%> (-0.03%)	⬇️
nilearn/glm/first_level/hemodynamic_models.py	98.58% <ø> (ø)
nilearn/glm/model.py	78.74% <100.00%> (+0.34%)	⬆️
nilearn/glm/contrasts.py	92.22% <85.71%> (-0.16%)	⬇️
nilearn/glm/second_level/second_level.py	96.62% <85.71%> (+0.26%)	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[bthirion]

LGTM overall

[Remi-Gau]

I am getting a failure on Windows only, on this test. Anyone can spot the issue here?
@bthirion @jeromedockes @ymzayek

nilearn/nilearn/glm/tests/test_check_events_file_uses_tab_separators.py

Lines 89 to 98 in d990eda

	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)

FAILED nilearn/glm/tests/test_check_events_file_uses_tab_separators.py::test_binary_opening_an_image_error - AssertionError: Regex pattern did not match.
 Regex: 'The file does not seem to be a valid unicode text file.'
 Input: "('The values in the events file are not separated by tabs; please enforce BIDS conventions', WindowsPath('C:/Users/runneradmin/AppData/Local/Temp/pytest-of-runneradmin/pytest-2/test_binary_opening_an_image_e0/temp_img.gif'))"

[jeromedockes]

I am getting a failure on Windows only, on this test. Anyone can spot the issue here?

it seems the default encoding used on windows with a wester/american locale is more lenient than utf-8:

import io

img_data = (
    b"GIF87a\x01\x00\x01\x00\xe7*\x00\x00\x00\x00\x01\x01"
    b"\x01\x02\x02\x07\x08\x08\x08\x0b\x0b\x0b\x0c\x0c\x0c\r;"
)
buf = io.BytesIO(img_data)
stream = io.TextIOWrapper(buf, encoding="cp1252")
stream.readline() # OK (but garbage output)
buf = io.BytesIO(img_data)
stream = io.TextIOWrapper(buf, encoding="utf-8")
stream.readline() # UnicodeDecodeError

btw,

• the documentation for open says a ValueError will be raised if an encoding error occurs (by default). UnicodeDecodeError is a subclass of ValueError so this is consistent with the behavior. but AFAICT there is no guarantee at least in the documentation that a UnicodeDecodeError will be used
• I'm not sure this check is useful, it basically raises the same error as the one that would propagate if it wasn't caught. actually it is less informative because the one raised by open specifies which encoding it tried, and that information is dropped by the nilearn rewording. also it is incorrect, because it states "does not appear to be a valid unicode text file", whereas it may be a perfectly well encoded text file, but encoded with a different encoding than the locale's default, which is used here

[jeromedockes]

also looking at the git blame for this file, it seems the black-formatting commits still show up in the git blame

[Remi-Gau]

also looking at the git blame for this file, it seems the black-formatting commits still show up in the git blame

I know. 🙈

• Adding [ENH] add a .git-blame-ignore-revs to ignore formatting PR from git blame #3653 to the milestone for the next release.

[Remi-Gau]

• I'm not sure this check is useful, it basically raises the same error as the one that would propagate if it wasn't caught. actually it is less informative because the one raised by open specifies which encoding it tried, and that information is dropped by the nilearn rewording. also it is incorrect, because it states "does not appear to be a valid unicode text file", whereas it may be a perfectly well encoded text file, but encoded with a different encoding than the locale's default, which is used here

Thanks for looking this up. I did not even where to look on this one.
So you would suggest nuking this test out of the test suite?

[jeromedockes]

Thanks for looking this up. I did not even where to look on this one.

when you see different encoding errors or absence thereof on windows and unix, it helps to remember they don't use the same default encoding another difference that comes up often in nilearn tests is windows refuses to unlink files that have an open file handle

So you would suggest nuking this test out of the test suite?

the check in the code and the test

[ymzayek]

LGTM!

[bthirion]

LGTM, thx !

[Remi-Gau]

Will fix the error in the doc build in this PR: it is just a typo (apparently I cannot spell my own name).