Skip to content
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

Nf mtms csd model #1168

Closed
wants to merge 5 commits into from

Conversation

@MrBago
Copy link
Contributor

commented Dec 25, 2016

I've implemented the multi-tissue multi-shell CSD model and wanted to push it upstream.

@MrBago

This comment has been minimized.

Copy link
Contributor Author

commented Dec 25, 2016

I realized that I left out the code for making the response function. For my project I was building the response function from using freesurfer segmentation. I've put that code into a gist, https://gist.github.com/MrBago/a2e141a0b2a9bcb772e4e59a22daefba.

Also when I use this model, I also use the parallel framework (#642) to make it faster. I believe @sahmed95 is working on getting that work ready to merge.

@coveralls

This comment has been minimized.

Copy link

commented Dec 25, 2016

Coverage Status

Coverage increased (+0.07%) to 88.469% when pulling 653504c on MrBago:nf_mtms-CSD_model into 7a89ef1 on nipy:master.

@codecov-io

This comment has been minimized.

Copy link

commented Dec 25, 2016

Current coverage is 85.95% (diff: 95.11%)

Merging #1168 into master will increase coverage by 0.07%

@@             master      #1168   diff @@
==========================================
  Files           213        215     +2   
  Lines         25834      26054   +220   
  Methods           0          0          
  Messages          0          0          
  Branches       2641       2649     +8   
==========================================
+ Hits          22187      22396   +209   
- Misses         2997       3004     +7   
- Partials        650        654     +4   

Powered by Codecov. Last update 7a89ef1...73c1abf

@MrBago MrBago force-pushed the MrBago:nf_mtms-CSD_model branch from 77243fe to 73c1abf Dec 25, 2016

@coveralls

This comment has been minimized.

Copy link

commented Dec 25, 2016

Coverage Status

Coverage increased (+0.07%) to 88.47% when pulling 77243fe on MrBago:nf_mtms-CSD_model into 7a89ef1 on nipy:master.

@coveralls

This comment has been minimized.

Copy link

commented Dec 26, 2016

Coverage Status

Coverage increased (+0.07%) to 88.47% when pulling 73c1abf on MrBago:nf_mtms-CSD_model into 7a89ef1 on nipy:master.

@MrBago

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2017

Hi all. I know there is a release coming up and I think this is a really solid model we should include in the next release if we can. Let me know if there is anything I can do to help streamline the review of this PR.

@arokem

This comment has been minimized.

Copy link
Member

commented Mar 6, 2017

Hey @MrBago : one way to help is to take a look at #1180, which is exploring a possible replacement for cvxopt. This will also affect what you are doing here, obviously.

@arokem

This comment has been minimized.

Copy link
Member

commented Apr 13, 2017

Hey @MrBago : any more thoughts about my comment (using cvxpy instead of using cvxopt)?

@MrBago

This comment has been minimized.

Copy link
Contributor Author

commented Apr 14, 2017

I should have time next weekend and I think I can get it done then.

@GuillaumeTh

This comment has been minimized.

Copy link
Contributor

commented Jul 17, 2017

Hi all, I have a problem with the PR, I test it with the peaks_from_model function and the computing is really long (more than 24 hours for a dwi in 1mm isotropic).

I have test in 2 processes and it's more fast than 8 processes. I think we have a problem with the multithreading. Maybe a variable in concurency.

Moreover, we must to set the variable OMP_NUM_THREADS to 1 or another number else cvxopt create other threads like numpy.

@MrBago

This comment has been minimized.

Copy link
Contributor Author

commented Jul 17, 2017

@MrBago

This comment has been minimized.

Copy link
Contributor Author

commented Jul 17, 2017

@Garyfallidis

This comment has been minimized.

Copy link
Member

commented Jul 24, 2017

The obvious question here is how much the original implementation takes and what libraries are used (if faster). The second question is where is the actual bottleneck (what takes most of the time).
Third question is where is the tutorial to help the reviewing process.
Also, the optimizer needs to change to cvxpy.

It would be great to have this merged for the next release. So, let's try to address the different issues here and move forward. Thanks.

@GuillaumeTh

This comment has been minimized.

Copy link
Contributor

commented Oct 2, 2017

Hi guys, I try to compute msmt fodf on a data which have a 0.3 mm isotropic of voxel size and I have an error. Have you any ideas ?

Thanks

@MrBago

This comment has been minimized.

Copy link
Contributor Author

commented Oct 2, 2017

@arokem

This comment has been minimized.

Copy link
Member

commented Oct 2, 2017

BTW - any chance to rewrite this using cvxpy?

@GuillaumeTh

This comment has been minimized.

Copy link
Contributor

commented Oct 3, 2017

@MrBago Sorry ! It was an obvious error with my Response Function.

@arokem I can help to test the code. But I can't refactor the code with cvxpy.

@GuillaumeTh

This comment has been minimized.

Copy link
Contributor

commented Jan 23, 2018

@MrBago Is it ok if me and @jchoude create some PR on your branch to merge this PR ?

@arokem I will check to rewrite using cvxpy

@MrBago

This comment has been minimized.

Copy link
Contributor Author

commented Jan 23, 2018

@GuillaumeTh sorry I didn't get around to this sooner, feel free to make a PR against this branch or make a fresh PR against dipy. Let me know if I can be of any help, I'll try and make time to review if it'll be useful.

@GuillaumeTh

This comment has been minimized.

Copy link
Contributor

commented Jan 23, 2018

@arokem Have you an idea about the solver that I need to use for the fitting ?

@MrBago Have you any ideas about the constraints for cvxpy ?

@GuillaumeTh

This comment has been minimized.

Copy link
Contributor

commented Jan 25, 2018

Hi guys,

I refactor the code with cvxpy but I have a big problem:

If I use ECOS as solver, the unit test work but there is a problem with large scale number (cvxgrp/cvxpy#261, embotech/ecos#142). So, I can't solve for fodf.

If I use SCS as solver, the unit test didn't work because the difference with the ground truth is 2 decimals.

Want do you think about that ? I know it's important to use cvxpy but now I'm stuck

@MrBago

This comment has been minimized.

Copy link
Contributor Author

commented Jan 26, 2018

@arokem I saw that on #1180 there was some conversation about the licensing of the two solvers. Do you know of the top of your head are both the ECOS & SCS solvers compatible with the dipy licence?

@arokem

This comment has been minimized.

Copy link
Member

commented Jan 29, 2018

I think either are fine.

@GuillaumeTh

This comment has been minimized.

Copy link
Contributor

commented Feb 12, 2018

Hi,
Have you an idea how to refactor the fitting part ?

@rutgerfick have you an idea ? I saw your PR on mapmri...

@rutgerfick

This comment has been minimized.

Copy link
Contributor

commented Feb 12, 2018

Hi @GuillaumeTh, I am not familiar with this code. What is exactly the problem and in which lines?

@GuillaumeTh

This comment has been minimized.

Copy link
Contributor

commented Feb 12, 2018

In msd.py at line 218, the ECOS solver doesn't solve the problem when the number are to high (higher than 10e9).

Is it normal ?

@rutgerfick

This comment has been minimized.

Copy link
Contributor

commented Feb 13, 2018

Hi @GuillaumeTh, I'm not 100% sure what is happening exactly but my I'm guessing the number you are talking about is the cost function value?

If this is the case then I think your optimization is doing something you're not intending. Is it possible that your spherical harmonics coefficients are exploding due to lack of sufficient regularization?

@GuillaumeTh

This comment has been minimized.

Copy link
Contributor

commented Feb 13, 2018

Hi @rutgerfick I think cvxpy is not robust to do this type of solving. I tried other solver with the same data and they worked. Yesterday I tested OSQP a solver with an Apache license (like cvxpy) and the results are good and the computing time is better than cvxopt.

So I can refactor the code with OSQP. @arokem @Garyfallidis is it possible to use that ?

@rutgerfick

This comment has been minimized.

Copy link
Contributor

commented Aug 29, 2018

Hi @GuillaumeTh , perhaps a bit of a late response but I implemented the MSMT-CSD algorithm in Dmipy using the cvxpy solver and it seems to do a good job. Take a look at the code and example if you're interested.

@arokem

This comment has been minimized.

Copy link
Member

commented Mar 15, 2019

@ShreyasFadnavis : are you following up on this work? Could you open a new PR from your branch of this and then close this?

@Borda
Copy link
Contributor

left a comment

I am missing any documentation or comments, the definition of inputs/outputs

if have_cvxopt:
cvx.solvers.options['show_progress'] = False

sh_const = .5 / np.sqrt(np.pi)

This comment has been minimized.

Copy link
@Borda

Borda Apr 27, 2019

Contributor

looks like a constant, then rather use SH_CONST = .5 / np.sqrt(np.pi)

iso_d = [sh_const] * iso
return np.concatenate([iso_d, out])

delta_functions = {"basic":_basic_delta,

This comment has been minimized.

Copy link
@Borda

Borda Apr 27, 2019

Contributor

also probably a constant like...

@ShreyasFadnavis

This comment has been minimized.

Copy link
Member

commented Apr 27, 2019

@ShreyasFadnavis : are you following up on this work? Could you open a new PR from your branch of this and then close this?

Hi @arokem,

Yes, I will be following up on this work! I am facing some issues with porting a few lines to CVXPY but will have it done in the coming days.

You can go ahead and close this PR! I will create a new one soon!

@ShreyasFadnavis

This comment has been minimized.

Copy link
Member

commented Apr 27, 2019

I am missing any documentation or comments, the definition of inputs/outputs

Hello @Borda,

Thank you for these suggestions. I will incorporate these in my PR!

@ShreyasFadnavis

This comment has been minimized.

Copy link
Member

commented Jun 14, 2019

Thanks @arokem! Opening up a new PR shortly 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.