matteofrigo commented 4 years ago

This commit adds the possibility to correct the tortuosity constraint by taking into account the multi-tissue properties of the employed model. In order to do this, the new syntax of the set_tortuous_parameter method of the MultiCompartmentModel class is extended in such a way that it can take as input the S0 of the tissues modelled by the intra-cellular and the extra-cellular compartments. Backward compatibility is maintained.

This PR substitutes #84 .

rutgerfick commented 4 years ago

can you add a test to check if it works as expected?

rutgerfick commented 4 years ago

strangely enough the test is not being reported here now, but you can find the pipeline for your pull request here: https://travis-ci.org/github/AthenaEPI/dmipy/pull_requests

it just has some small pep8 errors and then it's ready to go

rutgerfick commented 4 years ago

alright fixed the travis - it turns out i had to turn it off and on again in the settings due to some update

matteofrigo commented 4 years ago

There are actually a lot of PEP8 issues. I fixed the blank line around the nested function, but PyCharm tells me there's still 55 warnings and 147 weak warnings in the modeling_framework file.

rutgerfick commented 4 years ago

aside from the pep stuff, the tests that you added are not passing at the moment (raises are not being raised)

matteofrigo commented 4 years ago

The tests I added are passing, but the ones that check the raise of errors if MIX is used on tortuosity-constrained models are not. Also, the parametric fod spherical mean model is not correctly created from the fitted model.

The issues are most probably both caused by how I designed the encapsulation of the T1_tortuosity function in the constraint, which reads as follows.

        def tort_aux_func(lpar, ivf, evf):
            return T1_tortuosity(lpar, ivf, evf, S0_intra, S0_extra)

        self.parameter_links.append([model, name, tort_aux_func, [
            self._parameter_map[lambda_par_parameter_name],
            self._parameter_map[volume_fraction_intra_parameter_name],
            self._parameter_map[volume_fraction_extra_parameter_name]]]
        )

MIX

The check of the MIX+tortuosity error is done by looking for the T1_tortuosity function at the third entry of one of the parameter links

       for link in self.parameter_links:
            if link[2] is T1_tortuosity:
                msg = "Cannot use MIX optimization when the Tortuosity "
                msg += "constraint is set in the MultiCompartmentModel. To "
                msg += "use MIX while imposing Tortuosity, set the constraint "
                msg += "in the DistributedModel step."
                raise ValueError(msg)

which is no longer valid since I used the auxiliary function.

A possible solution would be to transform the T1_tortuosity function into a callable class that is instantiated in the set_tortuous_parameter function. In this way, the test could simply check if link[2] is an instance of T1_tortuosity. This comes with the pain of re-adapting all the rest of the code. Alternatively, the s0-s specified in the st_tortuous_parameter function should be saved as fixed parameters of the model in such a way that they can be read with self._parameter_map[s0_<intra/extra>_parameter_name]. In this case, would it be sufficient to add the two parameters to the self._parameter_scales and self._parameter_ranges dictionaries and then fixing them to the passed values? Would this have some weird implications?

Parametric FOD model

How the use of the auxiliary function affects the parametric fod model error is still unclear to me. The test crashes when it tries to fit the fod model created from the result of the smt model.

        fod_model = smt_fit.return_parametric_fod_model(
                distribution=distribution_name, Ncompartments=1
        )

It seems that the inverted parameter map of fod_model is not able to map the key (None, 'partial_volume_1'), but I was not able to reconstruct the failure chain. The only thing that I observed is that there is no partial_volume_1 parameter in the model. The free parameters are SD1WatsonDistributed_1_G2Zeppelin_1_lambda_par, SD1WatsonDistributed_1_SD1Watson_1_mu, SD1WatsonDistributed_1_SD1Watson_1_odi, SD1WatsonDistributed_1_partial_volume_0.

Any idea of what's broken? The error message is reported here below.

  File "/user/mfrigo/home/miniconda3/envs/talon/lib/python3.7/unittest/case.py", line 59, in testPartExecutor
    yield
  File "/user/mfrigo/home/miniconda3/envs/talon/lib/python3.7/unittest/case.py", line 628, in run
    testMethod()
  File "/user/mfrigo/home/miniconda3/envs/talon/lib/python3.7/site-packages/nose/case.py", line 198, in runTest
    self.test(*self.arg)
  File "/home/mfrigo/miorepo/dmipy/dmipy/core/tests/test_fitted_model_properties.py", line 86, in test_parametric_fod_spherical_mean_model
    fitted_fod_model = fod_model.fit(scheme, data)
  File "/home/mfrigo/miorepo/dmipy/dmipy/core/modeling_framework.py", line 1213, in fit
    self, self.scheme, x0_, Ns, N_sphere_samples)
  File "/home/mfrigo/miorepo/dmipy/dmipy/optimizers/brute2fine.py", line 71, in __init__
    model, x0_vector.squeeze(), Ns, N_sphere_samples)
  File "/home/mfrigo/miorepo/dmipy/dmipy/optimizers/brute2fine.py", line 168, in precompute_signal_grid
    self.acquisition_scheme, self.parameter_grid)
  File "/home/mfrigo/miorepo/dmipy/dmipy/core/modeling_framework.py", line 1312, in simulate_signal
    E_2d[i] = self(acquisition_scheme, **parameters)
  File "/home/mfrigo/miorepo/dmipy/dmipy/core/modeling_framework.py", line 1384, in __call__
    acquisition_scheme_or_vertices, **parameters)
  File "/home/mfrigo/miorepo/dmipy/dmipy/distributions/distribute_models.py", line 389, in __call__
    return self.sh_convolved_model(acquisition_scheme, **kwargs)
  File "/home/mfrigo/miorepo/dmipy/dmipy/distributions/distribute_models.py", line 466, in sh_convolved_model
    kwargs
  File "/home/mfrigo/miorepo/dmipy/dmipy/distributions/distribute_models.py", line 224, in add_linked_parameters_to_parameters
    argument_name = self._inverted_parameter_map[argument]
KeyError: (None, 'partial_volume_1')

rutgerfick commented 4 years ago

I'll have a look tomorrow, I'm sure this is just some spaghetti stuff we need to manage

rutgerfick commented 4 years ago

these guys you can fix already though

./dmipy/core/modeling_framework.py:132:41: E126 continuation line over-indented for hanging indent
./dmipy/core/modeling_framework.py:133:41: E123 closing bracket does not match indentation of opening bracket's line
./dmipy/core/modeling_framework.py:140:41: E126 continuation line over-indented for hanging indent
./dmipy/core/modeling_framework.py:141:41: E123 closing bracket does not match indentation of opening bracket's line
./dmipy/core/modeling_framework.py:641:38: E126 continuation line over-indented for hanging indent
./dmipy/core/modeling_framework.py:1274:17: E126 continuation line over-indented for hanging indent
./dmipy/core/modeling_framework.py:1382:25: E126 continuation line over-indented for hanging indent
./dmipy/core/modeling_framework.py:1384:21: E122 continuation line missing indentation or outdented
./dmipy/core/modeling_framework.py:1389:29: E126 continuation line over-indented for hanging indent
./dmipy/core/modeling_framework.py:1391:25: E122 continuation line missing indentation or outdented
./dmipy/core/modeling_framework.py:1676:17: E126 continuation line over-indented for hanging indent
./dmipy/core/modeling_framework.py:1784:25: E126 continuation line over-indented for hanging indent
./dmipy/core/modeling_framework.py:1786:21: E122 continuation line missing indentation or outdented
./dmipy/core/modeling_framework.py:2225:25: E126 continuation line over-indented for hanging indent
./dmipy/core/tests/test_multi_tissue_models.py:103:80: E501 line too long (80 > 79 characters)
./dmipy/core/tests/test_multi_tissue_models.py:112:80: E501 line too long (80 > 79 characters)

rutgerfick commented 4 years ago

with respect to the MIX check, your suggestion to make T1 tortuosity a callable class is by far the easiest to do.

Just go ahead and make it a class that has the self parameters of the tissue responses fixed in the set_tortuous_parameter function.

The overhead of chagning this throughout the codebase should be very minimal.

rutgerfick commented 4 years ago

As for the second, the lines that is to blame is the following:

https://github.com/AthenaEPI/dmipy/blob/ff5067f2cad47fb21bcac381ff1c9fc744e66312/dmipy/core/fitted_modeling_framework.py#L498

https://github.com/AthenaEPI/dmipy/blob/ff5067f2cad47fb21bcac381ff1c9fc744e66312/dmipy/core/fitted_modeling_framework.py#L593

when switching between MC-SM to MC or MC-SH, it re-applies links that were applied in the MC-SM to each bundle in the MC or kernel in MC-SH. When you fix the T1tortuosity as a callable class then this should be resolved as well.

rutgerfick commented 4 years ago

be sure to check if the multi-processing still works after you changed the tortuosity def to a class - I seem to remember it was had some issues but perhaps I am wrong.

codecov-commenter commented 4 years ago

Codecov Report

Merging #98 into master will increase coverage by 0.25%. The diff coverage is 96.80%.

@@            Coverage Diff             @@
##           master      #98      +/-   ##
==========================================
+ Coverage   82.14%   82.39%   +0.25%     
==========================================
  Files          64       65       +1     
  Lines        5616     5698      +82     
  Branches      667      668       +1     
==========================================
+ Hits         4613     4695      +82     
- Misses        817      819       +2     
+ Partials      186      184       -2

Impacted Files	Coverage Δ
dmipy/core/acquisition_scheme.py	`78.19% <0.00%> (ø)`
dmipy/core/tests/test_fitted_model_properties.py	`100.00% <ø> (ø)`
dmipy/core/modeling_framework.py	`72.71% <91.17%> (+0.30%)`	:arrow_up:
dmipy/core/fitted_modeling_framework.py	`67.11% <100.00%> (ø)`
dmipy/core/tests/test_multi_tissue_models.py	`100.00% <100.00%> (ø)`
dmipy/distributions/distribute_models.py	`72.28% <100.00%> (+0.06%)`	:arrow_up:
...stributions/tests/test_distributed_model_raises.py	`100.00% <100.00%> (ø)`
dmipy/utils/tests/test_tortuosity_multi_tissue.py	`100.00% <100.00%> (ø)`
dmipy/utils/utils.py	`70.25% <100.00%> (+0.77%)`	:arrow_up:
dmipy/distributions/tests/test_bingham.py	`91.04% <0.00%> (ø)`
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update ff5067f...8065936. Read the comment docs.

coveralls commented 4 years ago

Pull Request Test Coverage Report for Build 699

122 of 125 (97.6%) changed or added relevant lines in 8 files are covered.
4 unchanged lines in 1 file lost coverage.
Overall coverage increased (+0.1%) to 83.424%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
dmipy/core/acquisition_scheme.py	0	1	0.0%
dmipy/core/modeling_framework.py	32	34	94.12%
<!--	Total:	122	125	97.6%	-->

Files with Coverage Reduction	New Missed Lines	%
dmipy/distributions/tests/test_bingham.py	4	86.3%
<!--	Total:	4		-->

Totals
Change from base Build 695:	0.1%
Covered Lines:	4879
Relevant Lines:	5698

AthenaEPI / dmipy

add multi tissue correction to tortuosity constraint #98

MIX

Parametric FOD model

Codecov Report

Pull Request Test Coverage Report for Build 699

💛 - Coveralls