[FIX] fix SimpleRegressionResults #4071
Conversation
|
👋 @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.
For new features:
For bug fixes:
We will review it as quick as possible, feel free to ping us with questions if needed. |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #4071 +/- ##
==========================================
+ Coverage 91.85% 91.96% +0.11%
==========================================
Files 145 145
Lines 16360 16358 -2
Branches 3424 3423 -1
==========================================
+ Hits 15027 15044 +17
+ Misses 792 771 -21
- Partials 541 543 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
nilearn/glm/_base.py
Outdated
| @@ -8,7 +7,7 @@ class BaseGLM(BaseEstimator, TransformerMixin, CacheMixin): | |||
| """Implement a base class \ | |||
| for the :term:`General Linear Model<GLM>`.""" | |||
|
|
|||
| @auto_attr | |||
| @property | |||
There was a problem hiding this comment.
I have no issue with the change, but could you explain what it means/implies ?
There was a problem hiding this comment.
As far as I can tell these decorators do the same thing but one comes from the standard library, so feels like it is more appropriate to use it.
Do you want me to mention it in the changelog?
There was a problem hiding this comment.
IIUC it's not exactly the same, as property doesn't store the value as an object attribute after initial call (see https://nipy.org/nibabel/reference/nibabel.onetime.html#auto-attr). While using property works as well it seems it could add function call overhead that could affect performance. You could time some tests that we already have to quickly compare one versus the other.
There was a problem hiding this comment.
IIUC it's not exactly the same, as property doesn't store the value as an object attribute after initial call (see https://nipy.org/nibabel/reference/nibabel.onetime.html#auto-attr). While using property works as well it seems it could add function call overhead that could affect performance. You could time some tests that we already have to quickly compare one versus the other.
You were right @ymzayek it would make the code quite a bit slower.
Ran the following script on this branch and on main.
On main I am at about 0.197 seconds per iteration.
On this branch it increases up to 0.943 seconds per iteration if I try to get the residuals 10 times.
So yeah will revert those decorators.
"""Benchmark for getting GLM residuals
Runs a first level GLM N_ITER time on dummy data
and gets the residuals n times (between 0 and N_ITER_RESIDUALS times).
Gets the time taken to for 1 iteration (fit GLM and get residual n times).
"""python
from nilearn._utils.data_gen import (
generate_fake_fmri_data_and_design,
)
from nilearn.glm.first_level import (
FirstLevelModel,
)
import time
def compute_residuals(mask, fmri_data, design_matrices):
model = FirstLevelModel(
mask_img=mask, minimize_memory=False, noise_model="ols"
)
model.fit(fmri_data, design_matrices=design_matrices)
return model.residuals
N_ITER = 20
N_ITER_RESIDUALS = 11
def main():
shapes, rk = [(20, 20, 20, 100)], 3
mask, fmri_data, design_matrices = generate_fake_fmri_data_and_design(
shapes, rk
)
for i in range(len(design_matrices)):
design_matrices[i][design_matrices[i].columns[0]] = 1
for n_iter_residuals in range(1, N_ITER_RESIDUALS):
t0 = time.monotonic_ns()
for _ in range(N_ITER):
model = FirstLevelModel(
mask_img=mask, minimize_memory=False, noise_model="ols"
)
model.fit(fmri_data, design_matrices=design_matrices)
# get the residuals several times to see if this makes a difference
for __ in range(n_iter_residuals):
model.residuals
t1 = time.monotonic_ns()
print(f"Get residuals {n_iter_residuals} times")
print(f"{(t1-t0)/10**9/N_ITER:0.3f} seconds per iteration")
if __name__ == "__main__":
main()
|
I think that this PR could also address #2971 |
|
closed by mistake |
Co-authored-by: Yasmin <63292494+ymzayek@users.noreply.github.com>
|
will run some benchmark to check |
|
Can be merged I think ? |
Changes proposed in this pull request:
from @Gilles86 PR: Fix SimpleRegressionResults nistats#174