[FIX] improve support for F statistics in compute_fixed_effects
#3203
Conversation
|
👋 @bthirion 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.
We will review it as quick as possible, feel free to ping us with questions if needed. |
Codecov Report
@@ Coverage Diff @@
## main #3203 +/- ##
==========================================
+ Coverage 91.79% 91.82% +0.02%
==========================================
Files 134 134
Lines 15772 15816 +44
Branches 3284 3306 +22
==========================================
+ Hits 14478 14523 +45
Misses 751 751
+ Partials 543 542 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 11 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
bthirion
force-pushed
the
fix-fixed-effects
branch
from
36a221f
to
739986c
Compare
bthirion
force-pushed
the
fix-fixed-effects
branch
from
49537c2
to
9c707df
Compare
|
This one has been ready for some time. Can't it be counted in the upcoming release ? |
|
@bthirion sure we can try to make it work. Can you rebase on main, resolve the conflicts, and add a whatsnew entry? Then we can give it a final review and merge. |
|
@bthirion |
|
@bthirion and @Remi-Gau I just realized this PR breaks backwards compatibility so I'm not sure we should rush it. The default behavior of |
nilearn/glm/tests/test_contrasts.py
Outdated
| # F test | ||
| XX1 = np.vstack((X1, X1)) | ||
| XX2 = np.vstack((X2, X2)) | ||
|
|
||
| Xw, Vw, tw, zw = _compute_fixed_effects_params( | ||
| [XX1, XX2], [V1, V2], dofs=[200, 200], | ||
| precision_weighted=False) | ||
| assert_almost_equal(Xw, 1.5 * XX1) | ||
| assert_almost_equal(Vw, 1.25 * V1) |
There was a problem hiding this comment.
note to self when refactoring the tests: split into smaller independent tests
nilearn/glm/contrasts.py
Outdated
|
|
||
|
|
||
| def _compute_fixed_effects_params(contrasts, variances, precision_weighted): | ||
| def _compute_fixed_effects_params(contrasts, variances, precision_weighted, | ||
| dofs): | ||
| """Compute the fixed effects t-statistic, contrast, variance, \ |
There was a problem hiding this comment.
if this can now handle F contrast, the doc string should be updated, no?
Remi-Gau
changed the title
compute_fixed_effects
|
@bthirion updated the name of the PR. May need a double check. |
|
Thx @bthirion for the updates! I'll give another review once the CI finishes. Note that the test coverage report might fail due to black being applied to whole files that we still didn't cover in our reformatting journey. I would have rather done that in a separate PR but I think in this case since you are the only one touching these files (at least no other PR by a non coredev is working on these files) we can let it be. WDYT @Remi-Gau ? Anyways long story short, in the case of codecov/project failing I wouldn't worry about it as long as the new lines of code you wrote are covered. |
Co-authored-by: Yasmin <63292494+ymzayek@users.noreply.github.com>
There was a problem hiding this comment.
I would have rather done that in a separate PR but I think in this case since you are the only one touching these files (at least no other PR by a non coredev is working on these files) we can let it be. WDYT @Remi-Gau ?
Good with me to do the fade to black in this PR.
But if we do that then we must update the black and pre-commit config by deleting from them the files involved in this PR:
Agreed that this should probably be tackled by checking what our blind spots are by looking at codecov reports: e.g. https://app.codecov.io/gh/nilearn/nilearn/blob/main/nilearn%2Fglm%2Fcontrasts.py |
|
SHould be good now. Thx for the feedback. |
|
The black failure is weird. |
|
When in doubt, my first line of defense is usually. pre-commit install
pre-commit run -a |
|
It may be useful adding the |
Co-authored-by: Remi Gau <remi_gau@hotmail.com>
closes #3194.
Still need more tests (for F tests).
Detecting some inconsistencies in dimension: se sometimes require effect sizes for t contrasts to have shape (1, n_voxels) while it would be more natural to have shape (n_voxels). I think that we should actually support both cases seamlessly.