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

WIP - NF parallel framework #642

Closed
wants to merge 9 commits into from

Conversation

@MrBago
Copy link
Contributor

commented May 5, 2015

Starting a PR in case we want to discuss any of this stuff.

@arokem

This comment has been minimized.

Copy link
Member

commented May 5, 2015

I noticed a few typos, so I will start with that

array[index] = result[key]


class MultiVoxelFuntion(object):

This comment has been minimized.

Copy link
@arokem

arokem May 5, 2015

Member

=> "Function"

self._serial(self, data, mask, *args, **kwargs)


class UpdateCallback(MultiVoxelFuntion):

This comment has been minimized.

Copy link
@arokem

arokem May 5, 2015

Member

Same here (oops)

return np.round(split_points).astype(int)


def call_remotly(parallel_function, *args, **kwargs):

This comment has been minimized.

Copy link
@arokem

arokem May 5, 2015

Member

=> "remotely"

return parallel_function._serial(*args, **kwargs)


class ParallelFunction(MultiVoxelFuntion):

This comment has been minimized.

Copy link
@arokem

arokem May 5, 2015

Member

=> Function

b = maxcumsum(data, mask, weights, return_cumsum=False)
assert("cumsum" not in b)

class BrokenFucntion(ParallelFunction):

This comment has been minimized.

Copy link
@arokem

arokem May 5, 2015

Member

=> BrokenFunction


def _main(self, data):
raise ValueError
brokenfucntion = BrokenFucntion()

This comment has been minimized.

Copy link
@arokem

arokem May 5, 2015

Member

Same here


_dipy_num_cpu = 1

def active_multithreading(num_cpu=None):

This comment has been minimized.

Copy link
@MarcCote

MarcCote May 8, 2015

Contributor

It think it should be "activate" instead of "active".

elif num_cpu > 0:
_dipy_num_cpu = num_cpu

def deactive_multithreading():

This comment has been minimized.

Copy link
@MarcCote

MarcCote May 8, 2015

Contributor

It think it should be "deactivate" instead of "deactive".

_dipy_num_cpu = 1

@contextmanager
def multitheading_on(num_cpu=None):

This comment has been minimized.

Copy link
@MarcCote

MarcCote May 8, 2015

Contributor

"multitheading" => "multithreading"

class MultiVoxelFuntion(object):
"""A function that can be applied to every voxel of a dMRI data set.
Subclass MultiVoxelFuntion and define the _main and _default_Values methods

This comment has been minimized.

Copy link
@MarcCote

MarcCote May 8, 2015

Contributor

_default_Values => _default_values

@MrBago

This comment has been minimized.

Copy link
Contributor Author

commented May 19, 2015

I made multi_voxel_fit use the parallel framework, I'm still working on some of the other things we talked about but wanted to share this part for you all to look over.

@gabknight I realized that making a peaks_from_fit using the framework might be a little tricky because the framework expects arrays as inputs. I haven't actually looked at it closely so I'm not sure how much of an issue it is. I'm happy to talk over potential changes if it is an issue.

@MrBago MrBago force-pushed the MrBago:nf_parallel_framework branch from 7eedefc to 0513c9d May 19, 2015

@Garyfallidis

This comment has been minimized.

Copy link
Member

commented Aug 1, 2015

Ping! Any progress with this? Should I start having a look or wait a bit more?

@Garyfallidis

This comment has been minimized.

Copy link
Member

commented Aug 1, 2015

Also @MrBago I haven't heard back from you about the tracking API updates and paper. Neither about the odf_deconv with Cholesky. I would appreciate some updates. But hey I know you are busy with PhD thesis. Just give us a quick summary when you can.

@quantshah

This comment has been minimized.

Copy link
Contributor

commented Sep 14, 2016

@arokem @Garyfallidis Hi, I pulled this branch and profiled the DSI fitting on my laptop (2 cores) and here are the results :

On master, without multiprocessing : 10 loops, best of 3: 65.8 ms per loop
Using multiprocessing : 10 loops, best of 3: 33.4 ms per loop

@MrBago has already implemented this in multi_voxel_fit such that any fitting which uses the multi_voxel decorator (such as DSI) will use the multiprocessing framework. I have a slightly better idea about this now and can discuss further.

So where do we go from here ? And what are the issues to address.

Some thoughts :

  1. Get more modules to use multi_voxel. (Currently the following modules use it - csedeconv, dsi, gqi, mapmri, shore). I don't know which other models are suited for voxel by voxel fitting.
  2. Find an efficient way to split chunks of voxels based on mask.
  3. Implement a switch for turning multiprocessing 'on' or 'off'.
@quantshah

This comment has been minimized.

Copy link
Contributor

commented Sep 14, 2016

Used the taiwan_ntu_dsi dataset

from dipy.data import fetch_taiwan_ntu_dsi, read_taiwan_ntu_dsi, get_sphere
from dipy.reconst.dsi import DiffusionSpectrumModel

img, gtab = read_taiwan_ntu_dsi()

data = img.get_data()
dsmodel = DiffusionSpectrumModel(gtab)
dataslice = data[:, :, data.shape[2] / 2]

%%timeit
dsfit = dsmodel.fit(dataslice)
@Garyfallidis

This comment has been minimized.

Copy link
Member

commented Oct 5, 2016

Hi @sahmed95 these are all good points. Do you have a new PR to look into? Do look into how chunks are used for DTI fitting. You may get some ideas there about how to use the mask. Are you thinking about using this technique for IVIM too? Or this will be paralleled like DTI?
See here https://github.com/nipy/dipy/blob/master/dipy/reconst/dti.py#L1195

@Garyfallidis

This comment has been minimized.

Copy link
Member

commented Oct 5, 2016

I suggest first make a PR with a current parallelization that you have. And then look into using the mask to improve performance even further. Start first with the things that can obviously use the parallel multivoxel fit and then try to see if also other models e.g. dti can work in the same way. There we use the chunk size but is that the best way? Should we use chunks here too? Let me know what you think about that.

Also we need to make sure that we can save the results after the parallel fit and create a peaks_from_fit function. But hey one step each time. Obviously the ideal would be to have one multi_voxel_fit that would work with all available models. And the million dollar question here is ... can this happen without any dramas?

@quantshah

This comment has been minimized.

Copy link
Contributor

commented Oct 23, 2016

@Garyfallidis I opened a new PR #1135

@MrBago MrBago referenced this pull request Dec 25, 2016

@MrBago MrBago force-pushed the MrBago:nf_parallel_framework branch from 4afd1e5 to 810c425 Dec 26, 2016

@coveralls

This comment has been minimized.

Copy link

commented Dec 26, 2016

Coverage Status

Coverage decreased (-0.03%) to 88.367% when pulling 810c425 on MrBago:nf_parallel_framework into 7a89ef1 on nipy:master.

@Garyfallidis

This comment has been minimized.

Copy link
Member

commented Feb 17, 2017

I believe this PR needs to close in favor of #1135.

@skoudoro

This comment has been minimized.

Copy link
Member

commented Sep 15, 2018

closing in favor of #1418

@skoudoro skoudoro closed this Sep 15, 2018

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