Add logL as model output #230
Conversation
- Also output log-likelkihood (logL) from model
|
Hello @adelavega, Thank you for updating! Cheers! There are no style issues detected in this Pull Request. 🍻 To test for issues locally, Comment last updated at 2020-04-08 22:12:04 UTC |
|
@effigies @jdkent I'm testing out adding Unfortunately, it was actually not available with nistats is archived so I can't get that fix upstream, so that custom function to retrieve it would have to live here for now. |
|
Okay, great the maps look good. Only other point is what they key should be. |
|
Thanks for investigating this @adelavega, and sorry it was more pain to implement than anticipated.
I don't think Your point still stands that But I did not do a diligent check to make sure the attributes from the inherited class (
So I would change: So any attributes that Finally, I also like |
fitlins/interfaces/nistats.py
Outdated
|
|
||
| # Manually compute log-likelihood | ||
|
|
||
| def _get_voxelwise_logL(flm): |
There was a problem hiding this comment.
I don't know if it would be easier, but you could also patch the FirstLevelModel class with a more correct _get_voxelwise_model_attribute function definition, and use FirstLevelModelPatch here.
The main benefit being if this change is pulled into nilearn/nistats, then you can just delete the patched class and use the "normal" one again and not potentially change how you are solving the problem (by defining a custom function _get_voxelwise_logL).
Overthinking this since your solution works and is also good (and ultimately fewer lines of code).
from nistats.regression import RegressionResults
from nistats.model import LikelihoodModelResults
class FirstLevelModelPatch(FirstLevelModel)
def _get_voxelwise_model_attribute(self, attribute,
result_as_time_series):
"""Transform RegressionResults instances within a dictionary
(whose keys represent the autoregressive coefficient under the 'ar1'
noise model or only 0.0 under 'ols' noise_model and values are the
RegressionResults instances) into input nifti space.
Parameters
----------
attribute : str
an attribute of a RegressionResults instance.
possible values include: resid, norm_resid, predicted,
SSE, r_square, MSE.
result_as_time_series : bool
whether the RegressionResult attribute has a value
per timepoint of the input nifti image.
Returns
-------
output : list
a list of Nifti1Image(s)
"""
# check if valid attribute is being accessed.
# CHANGING THIS LINE TO BE BETTER
all_attributes = set(dict(vars(LikelihoodModelResults)).keys()).union(dict(vars(RegressionResults)).keys())
possible_attributes = [prop
for prop in all_attributes
if '__' not in prop
]
if attribute not in possible_attributes:
msg = ("attribute must be one of: "
"{attr}".format(attr=possible_attributes)
)
raise ValueError(msg)
if self.minimize_memory:
raise ValueError(
'To access voxelwise attributes like '
'R-squared, residuals, and predictions, '
'the `FirstLevelModel`-object needs to store '
'there attributes. '
'To do so, set `minimize_memory` to `False` '
'when initializing the `FirstLevelModel`-object.')
if self.labels_ is None or self.results_ is None:
raise ValueError('The model has not been fit yet')
output = []
for design_matrix, labels, results in zip(self.design_matrices_,
self.labels_,
self.results_
):
if result_as_time_series:
voxelwise_attribute = np.zeros((design_matrix.shape[0],
len(labels))
)
else:
voxelwise_attribute = np.zeros((1, len(labels)))
for label_ in results:
label_mask = labels == label_
voxelwise_attribute[:, label_mask] = getattr(results[label_],
attribute)
output.append(self.masker_.inverse_transform(voxelwise_attribute))
return output
There was a problem hiding this comment.
I think subclassing things from nistats is probably going to be more effort than it's worth at this point, but worth thinking about making these contributions upstream to nilearn post-0.7.0.
@adelavega Can you define this function at the file-level, rather than nested inside this scope?
There was a problem hiding this comment.
Yeah, I can do that. I think that's easiest for now.
There was a problem hiding this comment.
Sounds good to me, I can reference an issue in nilearn.
Codecov Report
@@ Coverage Diff @@
## master #230 +/- ##
==========================================
+ Coverage 77.62% 77.87% +0.25%
==========================================
Files 18 18
Lines 1059 1071 +12
Branches 189 190 +1
==========================================
+ Hits 822 834 +12
Misses 149 149
Partials 88 88
Continue to review full report at Codecov.
|
|
Looks like tests need to be re-run, random installation failure |
|
Tests passing. Happy with this? |
|
Let me test locally once more. |
|
Actually let's hold off, @tyarkoni is checking out if this is the log-likelihood we want. |
|
OK It looks good |