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 - multiprocessing multi voxel fit #631

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@gabknight
Contributor

gabknight commented Apr 18, 2015

This is pretty straight forward. I did the multiprocessing in the same way as in peaks_from_model. This work with all models implementing the multi_voxel_fit.

My main concern is the new dependency to dill (https://pypi.python.org/pypi/dill). I needed dill to serialize the single voxel fit function and send it to subprocesses. I didn't found a way without. What do you suggest?

I had to put SillyModel and SillyFit out the test_..() fct, since they need to be imported in subprocesses.

thanks,
Gab

@MrBago

This comment has been minimized.

Contributor

MrBago commented Apr 20, 2015

Hi Gab, I think if we're going to add more multiprocessing to dipy we should do it in a way that allows us avoid repeating ourselves. Abstracting out the multiprocessing stuff has several advantages. First, we get more code re-use so hopefully it's easier in the long term. Second, I think it makes the code easier for users. Having shared multi-processing code means we can do something like:

dipy.config.use_parallel(cores=4)
model.fit(data)
peaks_from_model(...)

which I think is cleaner than:

model.fit(data, parallel=True, cores=4)
peaks_from_model(..., parallel=True, cores=4)

Finally it should be easier to test, optimize and maintain the parallel code if we can abstract it out. I wrote a prototype of how we might do it here, master...MrBago:nf_parallel_framework, please take a look and I'm happy to discuss this further over skype of email.

@gabknight

This comment has been minimized.

Contributor

gabknight commented Apr 20, 2015

Hi Bago, your prototype is interesting. I would be glad to help you putting this in place. I'm not expert in python multiprocessing and I would be happy to learn more on this topic. I found not trivial to use the multiprocessing tools for class functions.

For the records, I had in mind to replace the peaks_from_model() parallelism by parallel multi_voxel_fit function. Since the fitting is the most computationally intensive part, there won't be much time lost keeping only the fitting in parallel.

I guess we close this PR for now, and move forward with your prototype.

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Apr 20, 2015

Hi @MrBago and @gabknight. Great to see you both working on this important design issue. Let's try to exchange experience and make a beautiful and practical API for this. I will have a more careful look at both PRs asap. @gabknight don't close this PR yet please.

But just quickly to say that the new framework should allow for example in the concept of voxel reconstruction:

  1. Knowing the index of the voxel being processed. So, when linalg for example fails we should know the exact voxel that that happened.
  2. Log information and provide a system to print how much of the processing has already be done.
  3. Allow for extra maps to be used for the fit. For example during the SHORE fitting an additional FA map can be used to improve fitting. This is not available with the current design.
  4. Allow simultaneous fitting of neighbour information to improve fitting and provide the capability for total variation based fitting. See for example Eric's work on that aspect
    http://www.researchgate.net/publication/267338971_Spherical_deconvolution_of_multichannel_diffusion_MRI_data_with_non-Gaussian_noise_models_and_total_variation_spatial_regularization

Furthermore, it would be nice if the same multiprocessing framework can be used for simulations (dipy.sims.voxel). Because this was not available in Dipy they made their own parallelization in Nipype it would be much nicer if we can provide this functionality ourselves.
nipy/nipype#1085 (comment)

Because this is an important design issue and parallelization in Python is not exactly an easy problem. I think we will need to setup a quick hangout for this sooner or later. Maybe at some point next week?

@arokem

This comment has been minimized.

Member

arokem commented Apr 20, 2015

Also, to chime in here, you might want to draw some inspiration from the
work that @cpoetter did in our recent ISBI submission:

https://github.com/arokem/ISBI2015/blob/master/parallelization.py

It's fairly generic, and has some mechanisms in place for logging, e.g. to
calculate how much of the work has already been done. This design can be
used for both reconstruction models, as well as simulations, I believe.

I agree with @Garyfallidis that a discussion about all these different
options would be good, and a hangout some time next week is a good idea.

I've started a poll for scheduling. Please fill in your availability!

http://doodle.com/t64752r6ypzh7e4z

On Mon, Apr 20, 2015 at 8:45 AM, Eleftherios Garyfallidis <
notifications@github.com> wrote:

Hi @MrBago https://github.com/MrBago and @gabknight
https://github.com/gabknight. Great to see you both working on this
important design issue. Let's try to exchange experience and make a
beautiful and practical API for this. I will have a more careful look at
both PRs asap. @gabknight https://github.com/gabknight don't close this
PR yet please.

But just quickly to say that the new framework should allow for example in
the concept of voxel reconstruction:

  1. Knowing the index of the voxel being processed. So, when linalg for
    example fails we should know the exact voxel that that happened.
  2. Log information and provide a system to print how much of the
    processing has already be done.
  3. Allow for extra maps to be used for the fit. For example during the
    SHORE fitting an additional FA map can be used to improve fitting. This is
    not available with the current design.
  4. Allow simultaneous fitting of neighbour information to improve fitting
    and provide the capability for total variation based fitting. See for
    example Eric's work on that aspect

http://www.researchgate.net/publication/267338971_Spherical_deconvolution_of_multichannel_diffusion_MRI_data_with_non-Gaussian_noise_models_and_total_variation_spatial_regularization

Furthermore, it would be nice if the same multiprocessing framework can be
used for simulations (dipy.sims.voxel). Because this was not available in
Dipy they made their own parallelization in Nipype it would be much nicer
if we can provide this functionality ourselves.
nipy/nipype#1085 (comment)
nipy/nipype#1085 (comment)

Because this is an important design issue and parallelization in Python is
not exactly an easy problem. I think we will need to setup a quick hangout
for this sooner or later. Maybe at some point next week?


Reply to this email directly or view it on GitHub
#631 (comment).

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Apr 20, 2015

Sorry @arokem, a naive question here. Is doodle taking account for time zone changes? For example, for Monday I see that the earliest time that you are available is 9am and the latest is 17:00. Is this in my timezone or yours?

@arokem

This comment has been minimized.

Member

arokem commented Apr 20, 2015

I was hoping that it would adjust by time-zone, but it seems to have put
this in my time zone, so the times you are looking at are my availability
in PST. Sorry about that.

Ariel

On Mon, Apr 20, 2015 at 10:19 AM, Eleftherios Garyfallidis <
notifications@github.com> wrote:

Sorry @arokem https://github.com/arokem, a naive question here. Is
doodle taking account for time zone changes? For example, for Monday I see
that the earliest time that you are available is 9am and the latest is
17:00. Is this in my timezone or yours?


Reply to this email directly or view it on GitHub
#631 (comment).

@arokem

This comment has been minimized.

Member

arokem commented Apr 20, 2015

Helpful indeed, but apparently not something that can be changed in an
existing poll.

Use this one instead:

http://doodle.com/sr6ykpgwphf9d69b

On Mon, Apr 20, 2015 at 10:22 AM, Eleftherios Garyfallidis <
notifications@github.com> wrote:

Would that help?

http://support.doodle.com/customer/portal/articles/645367-how-can-i-schedule-an-event-with-people-in-different-time-zones-


Reply to this email directly or view it on GitHub
#631 (comment).

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Apr 20, 2015

Thanx

@arokem

This comment has been minimized.

Member

arokem commented Apr 23, 2015

OK - seems like we've settled on a couple of times that work for three of
us. @MrBago - it would be great if you could participate as well. Does
either Tuesday, the 28th at 11 AM, or Thursday, the 30th at noon (both PST)
work for you?

On Mon, Apr 20, 2015 at 10:43 AM, Eleftherios Garyfallidis <
notifications@github.com> wrote:

Thanx


Reply to this email directly or view it on GitHub
#631 (comment).

@MrBago

This comment has been minimized.

Contributor

MrBago commented Apr 23, 2015

Can we do Thursday at noon? That works for me.

Bago

On Thu, Apr 23, 2015 at 10:42 AM, Ariel Rokem notifications@github.com
wrote:

OK - seems like we've settled on a couple of times that work for three of
us. @MrBago - it would be great if you could participate as well. Does
either Tuesday, the 28th at 11 AM, or Thursday, the 30th at noon (both PST)
work for you?

On Mon, Apr 20, 2015 at 10:43 AM, Eleftherios Garyfallidis <
notifications@github.com> wrote:

Thanx


Reply to this email directly or view it on GitHub
#631 (comment).


Reply to this email directly or view it on GitHub
#631 (comment).

@arokem

This comment has been minimized.

Member

arokem commented Apr 23, 2015

Yes - that seems to work for everyone that has responded to that. It's set:
Thursday, April 30th at noon PST. I will set up a calendar event with the
hangout link. For now, the participants are @Garyfallidis, @gabknight,
@MrBago and myself, but If anyone more is interested in joining, please let
me know, so that I can share the hangout link with you.

On Thu, Apr 23, 2015 at 11:15 AM, Bago Amirbekian notifications@github.com
wrote:

Can we do Thursday at noon? That works for me.

Bago

On Thu, Apr 23, 2015 at 10:42 AM, Ariel Rokem notifications@github.com
wrote:

OK - seems like we've settled on a couple of times that work for three of
us. @MrBago - it would be great if you could participate as well. Does
either Tuesday, the 28th at 11 AM, or Thursday, the 30th at noon (both
PST)
work for you?

On Mon, Apr 20, 2015 at 10:43 AM, Eleftherios Garyfallidis <
notifications@github.com> wrote:

Thanx


Reply to this email directly or view it on GitHub
#631 (comment).


Reply to this email directly or view it on GitHub
#631 (comment).


Reply to this email directly or view it on GitHub
#631 (comment).

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Apr 30, 2015

Great meeting guys. Thx!

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Dec 30, 2015

Closing this in favour of #642

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