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

Athena mapmri #1153

Merged
merged 95 commits into from Dec 21, 2016

Conversation

Projects
None yet
5 participants
@arokem
Member

arokem commented Nov 25, 2016

(a rebased version of #740 )

@coveralls

This comment has been minimized.

coveralls commented Nov 25, 2016

Coverage Status

Coverage increased (+0.4%) to 88.382% when pulling 4b61621 on arokem:athena_mapmri into e87b327 on nipy:master.

@codecov-io

This comment has been minimized.

codecov-io commented Nov 25, 2016

Current coverage is 85.88% (diff: 92.88%)

Merging #1153 into master will increase coverage by 0.39%

@@             master      #1153   diff @@
==========================================
  Files           214        213     -1   
  Lines         24960      25834   +874   
  Methods           0          0          
  Messages          0          0          
  Branches       2528       2641   +113   
==========================================
+ Hits          21339      22187   +848   
- Misses         2989       2997     +8   
- Partials        632        650    +18   

Powered by Codecov. Last update a0ddd21...83917b5

@coveralls

This comment has been minimized.

coveralls commented Nov 26, 2016

Coverage Status

Coverage increased (+0.4%) to 88.383% when pulling 970ea5b on arokem:athena_mapmri into e87b327 on nipy:master.

@arokem

This comment has been minimized.

Member

arokem commented Nov 26, 2016

This is now rebased on top of master. I have also added test coverage in the mapmri module, so that it is now up to 97% (on my laptop; 93% on Travis). @rutgerfick : when you have a chance, could you please take a look? Also, if you have data that generates all of the different error codes, it would be very useful to have data from these voxels, to add to the tests. @Garyfallidis : what else remains to do here, from your point of view?

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Nov 28, 2016

@arokem we need to try it and test it with multi-shell data. And also have another check in the code. Then merge.

@arokem

This comment has been minimized.

Member

arokem commented Nov 28, 2016

@rutgerfick

This comment has been minimized.

Contributor

rutgerfick commented Nov 29, 2016

@arokem

This comment has been minimized.

Member

arokem commented Dec 1, 2016

yes - best to do that as a follow-up after we merge this one. It's already a bit of a beast.

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Dec 1, 2016

Agreed. I am super excited about this PR. Trying in it now with some HCP data. What is crucial is find datasets that really show the capability of the method.

S0=1., angles=[(0, 0), (90, 0)],
fractions=[35, 35], snr=20):
r""" Calculates the three-dimensional signal attenuation E(q) originating
from within a cylinder of radius R using the Soderman approximation [1]_.

This comment has been minimized.

@Garyfallidis

Garyfallidis Dec 1, 2016

Member

The underscore is fine here.

References
----------
.. [1]_ Söderman, Olle, and Bengt Jönsson. "Restricted diffusion in

This comment has been minimized.

@Garyfallidis

Garyfallidis Dec 1, 2016

Member

But not needed here (I mean the underscore). I believe you are doing the same in many areas. I am not sure if it valid and perhaps the tutorial will not render correctly.

This comment has been minimized.

@rutgerfick

rutgerfick Dec 2, 2016

Contributor

Hi Elef, so to clarify, I need to use the underscore only in the 'text' and not when i define the citation?

This comment has been minimized.

@Garyfallidis

This comment has been minimized.

@rutgerfick

rutgerfick Dec 4, 2016

Contributor

Just a question, but why do you automatically put S0 to an arbitrary value that is not 1?
I saw it sometimes as 150 or 100 for default. Why not by default set it to 1 and keep the signal attenuation convention instead of unnormalized signal?

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Dec 1, 2016

I am currently getting this error

/home/elef/Devel/dipy/dipy/reconst/mapmri.py:357: UserWarning: The MAPMRI positivity constraint depends on CVXOPT (http://cvxopt.org/). CVXOPT is licensed under the GPL (see: http://cvxopt.org/copyright.html) and you may be subject to this license when using the positivity constraint.
  warn(w_s)
Traceback (most recent call last):
  File "reconst_mapmri.py", line 143, in <module>
    mapfit_positivity_aniso = map_model_positivity_aniso.fit(data_small)
  File "/home/elef/Devel/dipy/dipy/reconst/multi_voxel.py", line 33, in new_fit
    fit_array[ijk] = single_voxel_fit(self, data[ijk])
  File "/home/elef/Devel/dipy/dipy/reconst/mapmri.py", line 395, in fit
    cvxopt.solvers.options['show_progress'] = False
AttributeError: 'module' object has no attribute 'solvers'

Do I need to upgrade cvxopt?

@arokem

This comment has been minimized.

Member

arokem commented Dec 1, 2016

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Dec 2, 2016

No it seems the code is not compatible with the python-cvxopt version available in Ubuntu 16.04. When I downloaded cvxopt with pip and compiled it then it did work.

@arokem

This comment has been minimized.

Member

arokem commented Dec 2, 2016

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Dec 2, 2016

1.1.4 (ubuntu 16.04) vs 1.1.9 (pip)

References
----------
.. [1]_ Söderman, Olle, and Bengt Jönsson. "Restricted diffusion in

This comment has been minimized.

@rutgerfick

rutgerfick Dec 4, 2016

Contributor

Just a question, but why do you automatically put S0 to an arbitrary value that is not 1?
I saw it sometimes as 150 or 100 for default. Why not by default set it to 1 and keep the signal attenuation convention instead of unnormalized signal?

@rutgerfick

Ah cool! I didn't even know you needed to test the assert_raises.

@rutgerfick

This comment has been minimized.

Contributor

rutgerfick commented Dec 4, 2016

As for the specific data for the errorcodes. In practice it generally happens when background or skull voxels are included in the fitting, causing the positivity constraint to not converge or resulting in some non-invertible observation matrix.

Adding a test for this would probably work by trying to fit random noise and it will most likely break down.
In real data, the errorcode is also useful to figure out which voxels mess up, because typically you don't get any indication of where it breaks if it breaks from the multi-voxel fit.

@rutgerfick

This comment has been minimized.

Contributor

rutgerfick commented Dec 4, 2016

As far as I can tell the implementation looks fine, as well as the tests.
I'm not sure how I can go into this new pull request and make changes to the underscore issue Elef brought up. Would it be something like this? http://stackoverflow.com/questions/14947789/github-clone-from-pull-request

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Dec 4, 2016

All you need to do is make a PR on top of Ariel's PR. This PR. You do not need to clone again.

@arokem

This comment has been minimized.

Member

arokem commented Dec 5, 2016

I made the underscore fixes on my branch.

@arokem

This comment has been minimized.

Member

arokem commented Dec 5, 2016

@rutgerfick : regarding data for the different error codes: it would be good to have specific examples of these failures. Do you have such data? Ideally, we would put data from a single voxel that corresponds to each error straight into our tests (like we do here, for example: https://github.com/nipy/dipy/blob/master/dipy/reconst/tests/test_ivim.py#L78) and verify that we always get the corresponding error to each of the error codes.

@arokem

This comment has been minimized.

Member

arokem commented Dec 5, 2016

@Garyfallidis : regarding CVXOPT versions: it looks like the solvers module was introduced in version 1.1.7 of the software (released in 2014). How should we handle this version requirement? I guess that when a user calls these functions, and cvxopt is triggered, we can check for that. For the time being, I have edited the documentation to reflect this minimal requirement.

@arokem

This comment has been minimized.

Member

arokem commented Dec 5, 2016

I've also added a test for the CVXOPT version. Unfortunately, it's rather brittle. This is because as far as I can tell, there is no way to infer the version of CVXOPT, except to check whether the software you have installed has the solvers sub-module. The software doesn't have anything like a version string stored anywhere that I could find it.

@coveralls

This comment has been minimized.

coveralls commented Dec 5, 2016

Coverage Status

Coverage increased (+0.4%) to 88.383% when pulling bd9192e on arokem:athena_mapmri into e87b327 on nipy:master.

@coveralls

This comment has been minimized.

coveralls commented Dec 5, 2016

Coverage Status

Coverage increased (+0.4%) to 88.38% when pulling 8757274 on arokem:athena_mapmri into e87b327 on nipy:master.

@coveralls

This comment has been minimized.

coveralls commented Dec 5, 2016

Coverage Status

Coverage increased (+0.4%) to 88.38% when pulling 8757274 on arokem:athena_mapmri into e87b327 on nipy:master.

arokem added some commits Nov 25, 2016

TST + RF Enable passing of None as laplacian weighting.
In which case, we rely on the default behavior of `generalized_crossvalidation_array`
RF: Check that we have a sufficiently recent CVXOPT.
Unfortunately, there doesn't seem to be a more reliable way to check the
version of CVXOPT, except to infer that if it doesn't have the `solvers`
sub-module, it is older than 1.1.7.

@arokem arokem force-pushed the arokem:athena_mapmri branch from 8757274 to 83917b5 Dec 20, 2016

@arokem

This comment has been minimized.

Member

arokem commented Dec 20, 2016

Rebased. @Garyfallidis : any more thoughts here?

@coveralls

This comment has been minimized.

coveralls commented Dec 20, 2016

Coverage Status

Coverage increased (+0.4%) to 88.399% when pulling 83917b5 on arokem:athena_mapmri into a0ddd21 on nipy:master.

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Dec 21, 2016

I am having a meeting with a postdoc on Thursday morning to look more deeply into the scientific parts of this work. But codewise this is ready and working with different datasets!!! We should try to find some higher resolution datasets to showcase the power of this method. Thanks @rutgerfick and all for contributing. Merging!

@Garyfallidis Garyfallidis merged commit b4140c6 into nipy:master Dec 21, 2016

4 checks passed

codecov/patch 92.88% of diff hit (target 85.49%)
Details
codecov/project 85.88% (+0.39%) compared to a0ddd21
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.4%) to 88.399%
Details

ShreyasFadnavis pushed a commit to ShreyasFadnavis/dipy that referenced this pull request Sep 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment