[ENH] pad t and F 'short' contrasts with zeros #4067

Remi-Gau · 2023-10-18T15:48:35Z

Closes Length of contrast vectors depends on high cutoff frequency #2396

Changes proposed in this pull request:

pad t and F constrast when they are too short
throw a warning when padding is done
throw error only when contrast are longer than the number of columns

github-actions · 2023-10-18T15:48:51Z

👋 @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.

Remi-Gau · 2023-10-18T15:51:02Z

mentioned in the associated issue, using the contrasts symbolically (eg. 'c1 - c2') in the example mentioned in the issue so avoid having to change contrast length when the high pass filter model is changed.

Remi-Gau · 2023-10-18T15:53:36Z

@bthirion I would not mind an early review on this.

I do not like that the warnings / errors that have to be thrown in different places, feels like there should be a single function checking contrast shape and doing the padding rather than changes in several places.

bthirion · 2023-10-18T16:35:00Z

You mean, one single function for t and F ?

bthirion · 2023-10-18T16:35:10Z

The PR looks good overall.

Remi-Gau · 2023-10-19T11:45:13Z

nilearn/glm/contrasts.py

+            con_val = pad_contrast(
+                con_val=con_val, theta=reg.theta, contrast_type=contrast_type
+            )


@bthirion
any reason why for F contrasts we do not rely on the FContrast methods and do something like

reg = regression_result[label_].Fcontrast(con_val)

the way we do it for T contrasts above?

IIRC, this is because we had to trick it somehow to support fixed effects on F contrasts.

ok this may require a comment if we can remember the why otherwise I could add a TODO to try to refactor this

I can add this to your PR.

Added a TODO for now in case we want to merge and tackle this in a separate PR.

Maybe rather in another PR.

codecov · 2023-10-19T12:57:08Z

Codecov Report

Merging #4067 (3a13951) into main (0817b96) will increase coverage by 0.09%.
Report is 5 commits behind head on main.
The diff coverage is 96.15%.

@@            Coverage Diff             @@
##             main    #4067      +/-   ##
==========================================
+ Coverage   91.50%   91.60%   +0.09%     
==========================================
  Files         143      143              
  Lines       16072    16091      +19     
  Branches     3339     3345       +6     
==========================================
+ Hits        14707    14740      +33     
+ Misses        820      804      -16     
- Partials      545      547       +2

Flag	Coverage Δ
macos-latest_3.10	`91.49% <96.15%> (+<0.01%)`	⬆️
macos-latest_3.11	`91.49% <96.15%> (+<0.01%)`	⬆️
macos-latest_3.12	`91.49% <96.15%> (?)`
macos-latest_3.8	`91.45% <96.15%> (+<0.01%)`	⬆️
macos-latest_3.9	`91.46% <96.15%> (+<0.01%)`	⬆️
ubuntu-latest_3.10	`91.49% <96.15%> (+<0.01%)`	⬆️
ubuntu-latest_3.11	`91.49% <96.15%> (+<0.01%)`	⬆️
ubuntu-latest_3.12	`91.49% <96.15%> (+<0.01%)`	⬆️
ubuntu-latest_3.8	`91.45% <96.15%> (+<0.01%)`	⬆️
ubuntu-latest_3.9	`91.46% <96.15%> (+<0.01%)`	⬆️
windows-latest_3.10	`91.44% <96.15%> (+<0.01%)`	⬆️
windows-latest_3.11	`91.44% <96.15%> (+<0.01%)`	⬆️
windows-latest_3.12	`91.44% <96.15%> (+<0.01%)`	⬆️
windows-latest_3.8	`91.40% <96.15%> (+<0.01%)`	⬆️
windows-latest_3.9	`91.40% <96.15%> (+<0.01%)`	⬆️

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

Files	Coverage Δ
nilearn/glm/contrasts.py	`92.38% <100.00%> (+0.03%)`	⬆️
nilearn/glm/model.py	`78.40% <100.00%> (ø)`
nilearn/glm/_utils.py	`90.37% <94.44%> (+0.62%)`	⬆️

... and 7 files with indirect coverage changes

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

Remi-Gau · 2023-10-20T09:56:49Z

TODO

update changelog

nilearn/glm/tests/test_second_level.py

ymzayek

LGTM overall!

ymzayek · 2023-10-23T09:20:59Z

nilearn/glm/_utils.py

+        Where q = number of :term:`contrast` vectors
+        and p = number of regressors.
+
+    theta theta of RegressionResults instances


can you add the type for theta here?

added the type but also tried to improve the doc string: let me know if this reads sufficiently well.

bthirion

LGTM, thx.

bthirion · 2023-10-23T20:26:25Z

nilearn/glm/contrasts.py

+            con_val = pad_contrast(
+                con_val=con_val, theta=reg.theta, contrast_type=contrast_type
+            )


Maybe rather in another PR.

* pad contrasts * refactor * rm test for warning * remove obsolete test * add test for error F contrast * add comment * add comment * update changelog * update changelog * improve doc string

pad contrasts

6ce99c4

refactor

04fc6a0

Remi-Gau commented Oct 19, 2023

View reviewed changes

Remi-Gau added 2 commits October 19, 2023 14:03

rm test for warning

ee91eea

remove obsolete test

c53c142

bthirion reviewed Oct 20, 2023

View reviewed changes

nilearn/glm/tests/test_second_level.py Show resolved Hide resolved

Remi-Gau marked this pull request as ready for review October 23, 2023 08:11

Remi-Gau added 5 commits October 23, 2023 10:35

add test for error F contrast

3ee4a72

add comment

47c11c9

add comment

5f01140

update changelog

224eda6

update changelog

0f0609d

ymzayek reviewed Oct 23, 2023

View reviewed changes

improve doc string

3a13951

bthirion approved these changes Oct 23, 2023

View reviewed changes

Remi-Gau merged commit 697f77d into nilearn:main Oct 24, 2023
30 of 31 checks passed

Remi-Gau deleted the contrast_pad branch October 24, 2023 08:51

Remi-Gau mentioned this pull request Oct 24, 2023

explain or refactor why F contrasts are handled differently than t contrasts #4077

Open

bthirion mentioned this pull request Jan 30, 2024

[DOC] update maintenance documentation for the releases #4250

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ENH] pad t and F 'short' contrasts with zeros #4067

[ENH] pad t and F 'short' contrasts with zeros #4067

Remi-Gau commented Oct 18, 2023

github-actions bot commented Oct 18, 2023

Remi-Gau commented Oct 18, 2023

Remi-Gau commented Oct 18, 2023

bthirion commented Oct 18, 2023

bthirion commented Oct 18, 2023

Remi-Gau Oct 19, 2023

bthirion Oct 19, 2023

Remi-Gau Oct 19, 2023

bthirion Oct 19, 2023

Remi-Gau Oct 23, 2023

bthirion Oct 23, 2023

codecov bot commented Oct 19, 2023 •

edited

Remi-Gau commented Oct 20, 2023 •

edited

ymzayek left a comment

ymzayek Oct 23, 2023

Remi-Gau Oct 23, 2023

bthirion left a comment

bthirion Oct 23, 2023

[ENH] pad t and F 'short' contrasts with zeros #4067

[ENH] pad t and F 'short' contrasts with zeros #4067

Conversation

Remi-Gau commented Oct 18, 2023

github-actions bot commented Oct 18, 2023

Remi-Gau commented Oct 18, 2023

Remi-Gau commented Oct 18, 2023

bthirion commented Oct 18, 2023

bthirion commented Oct 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 19, 2023 • edited

Codecov Report

Remi-Gau commented Oct 20, 2023 • edited

TODO

ymzayek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bthirion left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 19, 2023 •

edited

Remi-Gau commented Oct 20, 2023 •

edited