First level residuals #410
Conversation
Codecov Report
@@ Coverage Diff @@
## master #410 +/- ##
==========================================
+ Coverage 92.66% 92.69% +0.03%
==========================================
Files 40 40
Lines 4514 4588 +74
==========================================
+ Hits 4183 4253 +70
- Misses 331 335 +4
Continue to review full report at Codecov.
|
|
I believe this is ready for review, @Gilles86, could you take a look to make sure I didn't fundamentally alter your pull request? |
|
Very cool. Will try to have a look this week. |
|
Hi @jdkent thanks for looking after this Feature. |
|
@bthirion I know we avoid using |
Which is why the |
- add docstrings - re-add @setattr_on_read - change rsq to r_square
|
Except for the codecov patch error, I think this is ready for review. I personally find patch coverage to be a rather noisy measure of a pull request, I have a very lenient setting in one of my own projects. But if the patch diff is important to fix, I can add a couple new tests so that "more lines" are covered for this pull request. |
Why is that preferable to a |
|
@bthirion best we wrap this one up before holidays. |
There was a problem hiding this comment.
Hey guys,
Great work. I toyed around a bit with this branch. For me it seems to work super well! Super nice to quickly make a nifti with predicted time series that you can view in Python without resorting to command line FSL stuff... :)
This will create confusion for users, if they access the method as an attribute once & then watch it fail the second time.
This does not seem to happen for me. Can you give an example when this happens?
One final thing: I had to struggle for 20 minutes to understand why the predicted time series were scaled completely off compared to real fMRI data. It turns out that FirstLevelModel standardizes the data across time by default. I have to say I personally don't like that, because if you don't know this, it's very hard to understand to know what's going on... Any thoughts about this?
-Gilles
No strong opinion. using setattr_on_read is fine. it is a caching mechanism so it could also be an option to use the firstlevelobject's joblib memory for this. it would also be perfectly fine (perhaps best) to just have a get_residuals() function and let the user hold a reference to the result if they want. is this going to be showcased in an example? |
|
Okay
That would be beyond the purview of this PR I think. |
still thinking about the upcoming merge into nilearn: if this were nilearn we would probably ask for this to be shown in an example |
|
I can have a go at it.
…On Wed, Dec 18, 2019 at 11:20 AM jeromedockes ***@***.***> wrote:
This seems nearly ready.
still thinking about the upcoming merge into nilearn: if this were nilearn
we would probably ask for this to be shown in an example
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#410>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAIPRTSBFQKUYFXZYWBU3YDQZH2QNANCNFSM4JPMYOJA>
.
--
Gilles de Hollander
Postdoctoral Researcher
University of Zurich
Department of Economics
website <http://www.gillesdehollander.nl/> | github
<https://github.com/Gilles86/>
|
|
also making that error message : https://github.com/nistats/nistats/pull/410/files#r359257721 a bit more verbose would be great -- if that is confusing for developers it certainly will be for users. but I can also do it in another PR if necessary |
| from nistats.reporting import get_clusters_table | ||
| from nistats.first_level_model import FirstLevelModel | ||
| import pandas as pd | ||
| import numpy as np |
There was a problem hiding this comment.
If you don't mind, could you split the import so that novel users are not overwhelmed by the ten initial lines of imports.
| cluster_threshold=20).set_index('Cluster ID', drop=True) | ||
| table.head() | ||
|
|
||
| masker = input_data.NiftiSpheresMasker(table.loc[range(1, 7), ['X', 'Y', 'Z']].values) |
There was a problem hiding this comment.
Can the corresponding locations be plotted ? Otherwise, this looks a bit cryptic
Also, you probably want to explain what you're doing here.
|
|
||
| Parameters | ||
| ---------- | ||
| attribute : str |
There was a problem hiding this comment.
attribute is defined within a fixed dictionary of values I guess ? This should be provided if the function is meant to be used publicly.
There was a problem hiding this comment.
I believe this should really be a private method (I've changed it to be a private method), but I added additional documentation and an internal check to be explicit anyway.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
nistats/first_level_model.py
Outdated
| @@ -583,6 +586,96 @@ def compute_contrast(self, contrast_def, stat_type=None, | |||
|
|
|||
| return outputs if output_type == 'all' else output | |||
|
|
|||
| def _get_voxelwise_model_attribute(self, attribute, result_as_time_series=True): | |||
There was a problem hiding this comment.
Private methods and functions should not have default args. Only public ones should. It's best to explicitly supply the arg for each parameter of private functions and methods.
| @@ -276,6 +276,7 @@ def __init__(self, theta, Y, model, wY, wresid, cov=None, dispersion=1., | |||
| dispersion, nuisance) | |||
| self.wY = wY | |||
| self.wresid = wresid | |||
| self.wdesign = model.wdesign | |||
|
|
|||
| @setattr_on_read | |||
| def resid(self): | |||
There was a problem hiding this comment.
@bthirion We should deprecate this and replace it with residuals in a separate PR, and others like it.
Or I should ask JD Kent to revert the changes I asked him to make and rename the resid mehotd in this PR back to resid so it stay consistent.
I like the former idea, renaming all uses of resid to residuals.
Does Nilearn have resid or residuals?
There was a problem hiding this comment.
Yes, let's do it in a sperate PR.
I'm not aware of resid in nilear? We should go for residuals
There was a problem hiding this comment.
Don't think Nilearn has any resid method.
There was a problem hiding this comment.
so we will keep the method as resid for this pull request, I will not change this
nistats/regression.py
Outdated
| Davidson and MacKinnon 15.2 p 662 | ||
| """ | ||
| return self.resid(Y) * positive_reciprocal(np.sqrt(self.dispersion)) | ||
| raise ValueError('minimize_memory should be set to False to make residuals or predictions.') |
There was a problem hiding this comment.
@bthirion @jdkent @Gilles86
I'm still confused about the suitability of this, and other such methods. It feels like logL(), resid() and norm_resid() are in effect, traps; waiting for unsuspecting users to call them and watch their programs fail.
If these methods do not have anything to say, then we should simply remove all of them, or convert this into a message rather than an error.
There was a problem hiding this comment.
I believe the motivation for this was to provide a hint to the user on how to get residuals from their model instead of an attribute error, but I see this check is handled here. I'm fine with removing those methods.
|
This seems ready to merge. Anything else @bthirion ? |
|
@jdkent Add whats_new entry and also update your name in the .mailmap . |
|
Thank you! Nistats is one of my favorite python neuroimaging packages. Keep up the good work! |
…me-resid * 'master' of https://github.com/nistats/nistats: First level residuals (nilearn#410) Replace numpy's deprecated dec with pytest's skipif (nilearn#423)
…ove-reporting * 'master' of https://github.com/nistats/nistats: Prevent duplication & insertion of reports from damaging website layout (nilearn#429) Fix whatsnew and the website's News/updates section (nilearn#428) GLM Reports: Computed masks are used if there is no user supplied one (nilearn#424) First level residuals (nilearn#410)
Continuation of PR #298