-
Notifications
You must be signed in to change notification settings - Fork 27
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
Multiprocess fitting script #80
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did my best reviewing. Not 100% sure what everything does, but it seems good to me. good that you have added unit testing too :).
@@ -75,52 +24,162 @@ def loop_over_first_n_minus_1_dimensions(arr): | |||
flat_view = arr[idx].flatten() | |||
yield idx, flat_view | |||
|
|||
def generate_data(data, bvals, b0_indices, groups, total_iteration): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1: Is b0_indices something you want to tell the algorithm, or is it more fool proof if we generate them locally from bvals?
2: Maybe we need to start documenting some code; for example, to me "generate_data" sounds like it will generate data, but it is unclear why it needs data for this (if there is already data, why generate it?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The source could come from anywhere, this splits the data up based on the bvals and directions. The b0 indices are included in order to give the b0 data to every fit.
True, I should update it with some more documentation.
yield (data[idx, groups[:, dir]].flatten(), bvals[:, groups[:, dir]].ravel(), b0_indices[:, groups[:, dir]].ravel()) | ||
|
||
def osipi_fit(fitfunc, data_bvals): | ||
data, bvals, b0_indices = data_bvals |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make more sense to use dictionaries for these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think Ivan has a PR for that so maybe there's a conflict between these. I guess either his or mine should get merged first and then the other will need to update the other.
indices = index[bvals_inverse_sort] | ||
return shells, indices, indices<atol #, bvals_indices | ||
|
||
def find_directions(bvecs_raw, zero_indices, atol=1, rotationally_symmetric=True): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Are we doing a lot with vectors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really right now. I was thinking some algorithms will want to operate on a single direction at a time so that's why I wrote this, to be able to automatically split those up.
Thanks, I'll attempt to implement a version of this in the wrapper |
I wasn't trying to get you to work on it, just throwing this out as an initial implementation. I do think adding it to the wrapper is the best approach though. I was thinking of putting it in the wrapper like you did the multi voxel fit. Probably each algorithm should implement their own data generator though. I need to think about the structure a little more. |
Describe the changes you have made in this PR
This adds multiprocess fitting to the nifti fitting script. Perhaps better would be to add it to the wrapper. This is mostly an example. It could be added but is mostly opened for suggestions.
Checklist