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

Constrained optimisation for SHORE to set E(0)=1 when the CVXOPT package is available #324

Merged
merged 40 commits into from Mar 13, 2014

Conversation

Projects
None yet
9 participants
@demianw
Contributor

demianw commented Feb 10, 2014

ENH: Added three functionalities to SHORE:

  1. Constrained optimization for E(0)=1
  2. Outputting the fitted signal
  3. Compute the signal E(q) in the gradients when masked by b0s_mask as E(q)=0

Garyfallidis and others added some commits Jan 21, 2014

Merge pull request #319 from Garyfallidis/promo_dipy_paper
DOC: Updated the highlights to promote the release and the upcoming paper
ENH: Added two functionalities to SHORE: 1) Constrained optimization …
…for E(0)=1 2) Outputting the fitted signal
@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Feb 10, 2014

Demian, what is the difference in performance when using cvxopt vs the standard version?

Also, if cvxopt does a better job then there should be a warning message recommending the user to install it.

@demianw

This comment has been minimized.

Contributor

demianw commented Feb 10, 2014

Hi Eleftherios,

For 288 samples in q space and a single voxel

pseudoinverse method: ~ 1ms
constrained CVXOPT version with E(0)=1: ~ 0.7 ms

I didn't want to put the warning like that cause I don't know what is the DIPY criterium with respect to adding more dependencies. Also, I don't think that dividing the whole set of coefficients by the E(0) value before normalisation is mathematically sound as it reweighs completely all the signal. However, I have not made all the calculations to support this hypothesis

@ecaruyer

This comment has been minimized.

Contributor

ecaruyer commented Feb 10, 2014

Hi Demian!

Depending on how cvxopt (or the functions it wraps) are implemented, it might be overkill to use quadratic programming for this problem - since we only have equality constraints. There actually is an analytical solution to this question of constrained fitting, enforcing E(0) = 1. You may want to checkout [1], where I derive the eqs for the SPF basis. Since SHORE is a subset of SPF, things should translate easily to SHORE.

  1. Caruyer, Emmanuel, and Rachid Deriche. "Diffusion MRI signal reconstruction with continuity constraint and optimal regularization." Medical Image Analysis (2012). http://dx.doi.org/10.1016/j.media.2012.06.011
@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Feb 10, 2014

Thx @ecaruyer. @maurozucchelli and @mpaquette have been working on this problem too. You will need to coordinate guys!

@maurozucchelli

This comment has been minimized.

Contributor

maurozucchelli commented Feb 10, 2014

Hi @demianw I was also working on a PR that uses cvxopt on the SHORE model. I add linear constrain on the fitting in order to get a positive propagator (like Ozarslan 2013) and add new metrics. You can view my branch here https://github.com/maurozucchelli/dipy/tree/shore_metrics

@demianw

This comment has been minimized.

Contributor

demianw commented Feb 10, 2014

Hi @maurozucchelli , our approaches are complementary. We both use CVXOPT but to perform different tasks. The code overlap should be easy to solve. I see you are adding lots of good stuff! That's great. My philosophy of doing small very to-the-point pull requests is to enable these types of collaborations.

Also, have you solved how to add CVXOPT successfully to Travis? I've done it but

  1. Travis fails to build the pipy version of CVXOPT
  2. the version packaged in Debian seems to be older compared to the one in the pipy repository and that generates errors.
@maurozucchelli

This comment has been minimized.

Contributor

maurozucchelli commented Feb 10, 2014

I didn't made the pool request yet, but on my laptop I used the apt-get version of cvxopt.
The only problem that I had was with the computation time: adding positivity constraints make the computation last 40 minutes instead of 20 second for a single 128x128 slice.
Have you noticed the same thing? Do you have any advise?

@JianCheng

This comment has been minimized.

JianCheng commented Feb 10, 2014

Hi @demianw , E(0)=1 can be solved in a analytical way for both SHORE basis and SPF basis. Maybe you can check my thesis on it. See formula 5.70 in
http://tel.archives-ouvertes.fr/docs/00/75/90/48/PDF/thesis_JianCheng.pdf

@demianw

This comment has been minimized.

Contributor

demianw commented Feb 10, 2014

Hi @maurozucchelli , the positivity constraint is super fast in my implementation. I think that my best suggestion is to check my pull request.

@JianCheng, thanks, I will cheek it out. You mean that by carefully choosing the fitting problem we don't need to perform constrained optimisation and can solve it by least squares?

@JianCheng

This comment has been minimized.

JianCheng commented Feb 10, 2014

@demianw , yes. Since the constraint E(0)=1 is a linear equality constraint, we can separate the parameters into two parts and let one part called independet part to represent the other dependent part. Then the cost function becomes a least square only for the independent part. After estimating the independent part, the other dependent part is obtained based on the linear equality constraint.

For the non-negative constraint, this trick does not work.
But if we change the linear basis representation into square root representation, the non-negative constraint in continuous R^3 can be automatically inside the representation. But the problem is non-convex. A gradient descent with a good initialization is needed.
See my paper on non-negative EAP and ODF estimation.
"Nonnegative Definite EAP and ODF Estimation via a Unified Multi-shell HARDI Reconstruction", MICCAI 2012

@demianw

This comment has been minimized.

Contributor

demianw commented Feb 11, 2014

After the last push, it passes the tests for python 2.7, for python 3, the CVXOPT debian package is not readily available

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Feb 17, 2014

Hi Demian, It seems that there is python3-cvxopt for fedora but not for debian. Maybe we could download it and compile the source ourselves for python 3 or use another trick. Will have a look asap.

@demianw

This comment has been minimized.

Contributor

demianw commented Feb 17, 2014

Sounds good. I might have other small pull requests with features. Would you rather me add them like this in small precise pull requests or in big bundles of lots of stuff?

@arokem

This comment has been minimized.

Member

arokem commented Feb 17, 2014

Small and topic-specific beats large and amorphous every time! No PR is
ever too short to review :-)

On Mon, Feb 17, 2014 at 1:01 PM, Demian Wassermann <notifications@github.com

wrote:

Sounds good. I might have other small pull requests with features. Would
you rather me add them like this in small precise pull requests or in big
bundles of lots of stuff?


Reply to this email directly or view it on GitHubhttps://github.com//pull/324#issuecomment-35322473
.

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Feb 17, 2014

I would prefer in small PRs when possible.

But first please make sure for this PR that you have a good coordination with @maurozucchelli so that you don't reimplement the same functionality multiple times.

Go Go Team!!

matthieudumont and others added some commits Feb 24, 2014

Merge pull request #331 from mekman/fix_example
DOC: correct number of seeds in streamline_tools example
@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Mar 2, 2014

Hi @demianw and sorry for the delayed feedback.

It seems that if you change in .travis.yml into using
pip install cvxopt
travis will be happy.

Give it a go and let's see what happens! :)

arokem and others added some commits Mar 2, 2014

@demianw

This comment has been minimized.

Contributor

demianw commented Mar 3, 2014

Hi @Garyfallidis, I tried that. Sadly it doesn't work

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Mar 3, 2014

@matthew-brett any ideas?

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Mar 3, 2014

Build failure is here: https://travis-ci.org/nipy/dipy/jobs/19956807#L422

Looks like it needs the blas and lapack development libraries installed.

Maybe add apt-get install libblas-dev liblapack-dev ? Probably better to do this just for python 3, as in something like this in .travis.yml:

- if [ "${PYTHON}" == "python3" ]; then
     apt-get install libblas-dev liblapack-dev ;
     sudo pip$PYSUF install cvxopt ;
   else
     sudo apt-get install python-cvxopt ;
   fi
@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Mar 3, 2014

Sorry - missed some semi-colons in the code fragment above - I just added them.

@demianw demianw closed this Mar 12, 2014

@demianw demianw reopened this Mar 12, 2014

@demianw

This comment has been minimized.

Contributor

demianw commented Mar 12, 2014

Worked! Thanks @arokem !

@arokem

This comment has been minimized.

Member

arokem commented Mar 12, 2014

Cool! Other than that, is this ready to be merged? @maurozucchelli : did you have any other questions/comments? If we merge this, can this serve as a basis for you to keep working on your side of things?

@maurozucchelli

This comment has been minimized.

Contributor

maurozucchelli commented Mar 13, 2014

It is ok for me! When this will be merged I have another pool request ready that uses cvxopt to enforce positive constrains on the propagator, from the preliminary results it improves a lot the quality of the ODFs and the maps with noisy data. I'm also developing new maps but I think that is better to do a separate pool request for that...
Thanks a lot for your work to add cvxopt @demianw , @arokem and everybody!

arokem added a commit that referenced this pull request Mar 13, 2014

Merge pull request #324 from demianw/shore_e0_constrained_optimization
Constrained optimisation for SHORE to set E(0)=1 when the CVXOPT package is available

@arokem arokem merged commit f9a8e9c into nipy:master Mar 13, 2014

1 check passed

default The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment