-
Notifications
You must be signed in to change notification settings - Fork 32
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
FEAT: add skeleton for amico implementation #89
base: master
Are you sure you want to change the base?
Changes from 1 commit
3b8210a
1a72f0d
f12ce31
3fa0f81
090b123
66f67e6
6a101b2
4ef1dbe
0278fa1
b398592
873fb4e
4a0d277
02ad74e
85994d9
55b76fe
9958209
626444f
d1b7100
4e1a38d
08d74e1
735b2a1
99168c0
fe3f35c
6d65658
51aa0ab
692bb15
3cee2c8
307958c
0e1f5bf
70d56cf
e7a506b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2205,11 +2205,15 @@ class MultiCompartmentAMICOModel(MultiCompartmentModelProperties): | |
the models to combine into the MultiCompartmentModel. | ||
S0_tissue_responses: list of N values, | ||
the S0 response of the tissue modelled by each compartment. | ||
Nt: int | ||
Number of equidistant sampling points over each | ||
parameter range used to create atoms of observation matrix M | ||
parameter_links : list of iterables (model, parameter name, link function, | ||
argument list), | ||
deprecated, for testing only. | ||
""" | ||
def __init__(self, models, S0_tissue_responses=None, parameter_links=None): | ||
def __init__(self, models, S0_tissue_responses=None, Nt=10, | ||
parameter_links=None): | ||
self.models = models | ||
self.N_models = len(models) | ||
if S0_tissue_responses is not None: | ||
|
@@ -2219,6 +2223,7 @@ def __init__(self, models, S0_tissue_responses=None, parameter_links=None): | |
raise ValueError( | ||
msg.format(len(S0_tissue_responses), self.N_models)) | ||
self.S0_tissue_responses = S0_tissue_responses | ||
self.Nt = Nt | ||
self.parameter_links = parameter_links | ||
if parameter_links is None: | ||
self.parameter_links = [] | ||
|
@@ -2228,6 +2233,12 @@ def __init__(self, models, S0_tissue_responses=None, parameter_links=None): | |
self._prepare_parameter_links() | ||
self._prepare_model_properties() | ||
self._check_for_double_model_class_instances() | ||
|
||
# QUESTION: I think we should create multi-compartment model | ||
# somewhere in __init__ to avoid creating it every time we create | ||
# forward model matrix | ||
self.mc_model = MultiCompartmentModel(models=self.models) | ||
|
||
self._prepare_parameters_to_optimize() | ||
self._check_for_NMR_and_other_models() | ||
self.x0_parameters = {} | ||
|
@@ -2271,16 +2282,83 @@ def amico_idx(self): | |
matrix.""" | ||
return self._amico_idx | ||
|
||
def forward_model_matrix(self, *args, **kwargs): | ||
# TODO: move the creation of the forward model matrix from the optimizer | ||
# to here. At the same time, instantiate the parameter grid and | ||
# indices. | ||
# - Create the forward model matrix that will be returned | ||
# - Instantiate the self._amico_grid and self._amico_idx attributes | ||
# - Save the "freezed" parameters vector as a hidden attribute and | ||
# check if it has changed since the last call. | ||
# Attribute: self._freezed_parameters_vector | ||
raise NotImplementedError | ||
def forward_model_matrix(self, acquisition_scheme, model_dirs, **kwargs): | ||
"""Creates forward model matrix, including parameter tessellation grid | ||
and corresponding indices. | ||
""" | ||
""" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Double docstring. Just needs to be merged. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what do you mean? you just want to merge this in so you can continue on the other branch? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, actually that the two docstrings should be merged in a single docstring. |
||
Arguments: | ||
acquisition_scheme: instance containing acquisition protocol | ||
model_dirs: list containing direction of all models in | ||
multi-compartment model | ||
Returns: | ||
observation matrix M | ||
""" | ||
|
||
dir_params = [p for p in self.mc_model.parameter_names | ||
if p.endswith('mu')] | ||
if len(dir_params) != len(model_dirs): | ||
raise ValueError("Length of model_dirs should correspond " | ||
"to the number of directional parameters!") | ||
|
||
if not self._freezed_parameters_vector: | ||
self._amico_grid, self._amico_idx = {}, {} | ||
|
||
grid_params = \ | ||
[p for p in self.mc_model.parameter_names | ||
if not p.endswith('mu') and | ||
not p.startswith('partial_volume')] | ||
|
||
# Compute length of the vector x0 | ||
x0_len = 0 | ||
for m_idx in range(self.N_models): | ||
m_atoms = 1 | ||
for p in self.models[m_idx].parameter_names: | ||
if self.mc_model.model_names[m_idx] + p in grid_params: | ||
m_atoms *= self.Nt | ||
x0_len += m_atoms | ||
|
||
for m_idx in range(self.N_models): | ||
model = self.models[m_idx] | ||
model_name = self.mc_model.model_names[m_idx] | ||
|
||
param_sampling, grid_params_names = [], [] | ||
m_atoms = 1 | ||
for p in model.parameter_names: | ||
if model_name + p not in grid_params: | ||
continue | ||
grid_params_names.append(model_name + p) | ||
p_range = self.mc_model.parameter_ranges[model_name + p] | ||
self._amico_grid[model_name + p] = \ | ||
np.full(x0_len, np.mean(p_range)) | ||
param_sampling.append(np.linspace(p_range[0], p_range[1], | ||
self.Nt, endpoint=True)) | ||
m_atoms *= self.Nt | ||
|
||
self._amico_idx[model_name] =\ | ||
sum([len(self._amico_idx[k]) | ||
for k in self._amico_idx]) + \ | ||
np.arange(m_atoms) | ||
params_mesh = np.meshgrid(*param_sampling) | ||
for p_idx, p in enumerate(grid_params_names): | ||
self.grids[p][self._amico_idx[model_name]] =\ | ||
np.ravel(params_mesh[p_idx]) | ||
|
||
self._amico_grid['partial_volume_' + str(m_idx)] = \ | ||
np.zeros(x0_len) | ||
self._amico_grid['partial_volume_' + | ||
str(m_idx)][self._amico_idx[model_name]] = 1. | ||
self._freezed_parameters_vector = True | ||
|
||
for d_idx, dp in enumerate(dir_params): | ||
self._amico_grids[dp] = model_dirs[d_idx] | ||
|
||
# QUESTION: | ||
# Should we return simulated signals with b=0 values | ||
# or only diffusion weighted, I think b=0 impacts a lot | ||
# results when solving nnls | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why should it matter to nnls? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because b=0 have higher values than diffusion weighted values (and less affected by noise), so it will force that sum of b=0 values of atoms is equal to 1 and in this way will prevent that some atoms are fitted to noise. Also, there is a difference if we use for example 18 b=0 values (as in HCP dataset) or just single b=0 value that is equal to the mean of all b=0 values (that would be 1 after normalization), because the higher the number of b=0 values, nnls will give more importance to fitting b=0 parts of the atoms. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So you vote for keeping the b=0 measurements then, if I understand correctly? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with this also. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, but maybe it is something that we can investigate There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we can leave the default as being just sample anything that is in the acquisition scheme. |
||
return self.mc_model.simulate_signal(acquisition_scheme, | ||
self._amico_grid).T | ||
|
||
def fit(self, acquisition_scheme, data, | ||
mask=None, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1