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

DTI memory: use the same step in prediction as you use in fitting. #857

Merged
merged 5 commits into from Feb 9, 2016

Conversation

Projects
None yet
4 participants
@arokem
Member

arokem commented Feb 2, 2016

This should help us with the issues that are currently being discussed over on #840

I have not memory profiled this yet, though. This is what I intend to do next, so stay tuned. Would be great if others could try this out as well.

@MrBago

This comment has been minimized.

Contributor

MrBago commented Feb 3, 2016

@arokem could you do me a favor, while you're at this could you profile this for me too. It should also improve both the run time and the memory usage of tensor_predict.

@arokem

This comment has been minimized.

Member

arokem commented Feb 3, 2016

Will do.

I can also merge that commit into this branch, if that's OK with you.

On Tue, Feb 2, 2016 at 4:01 PM, Bago Amirbekian notifications@github.com
wrote:

@arokem https://github.com/arokem could you do me a favor, while you're
at this could you profile this
https://github.com/mrbago/dipy/tree/streamline_dti_predict for me too.
It should also improve both the run time and the memory usage of
tensor_predict.


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

@arokem

This comment has been minimized.

Member

arokem commented Feb 3, 2016

@arokem

This comment has been minimized.

Owner

arokem commented on ad567a1 Feb 3, 2016

This is great!

@arokem

This comment has been minimized.

Member

arokem commented Feb 3, 2016

This is memory profiling including @MrBago's additions (ad567a1): https://gist.github.com/arokem/a62377a861ca4b02db15

@arokem

This comment has been minimized.

Member

arokem commented Feb 3, 2016

I believe this is ready for a review.

@arokem arokem changed the title from WIP: DTI memory: use the same step in prediction as you use in fitting. to DTI memory: use the same step in prediction as you use in fitting. Feb 3, 2016

@samuelstjean

This comment has been minimized.

Contributor

samuelstjean commented Feb 4, 2016

It actually works now

In [35]: %memit data_p = tenfit.predict(gtab, S0)
/home/samuel/python/dipy/reconst/dti.py:1759: RuntimeWarning: divide by zero encountered in log
  D[..., 6] = -np.log(b0)

peak memory: 6689.07 MiB, increment: 5655.26 MiB

Might want to check that log(b0), just in case it threw off the profiler. A quick guess is that it does log(S0)? At least it does not blow up like before, so that's an improvement.

@arokem

This comment has been minimized.

Member

arokem commented Feb 4, 2016

Good to hear that it works for you.

I am not sure what you are asking about b0/S0. Looks like you have some voxels with the non diffusion-weighted signal at 0 (outside the brain, possibly). I am not sure how that would throw off the profiler.

@samuelstjean

This comment has been minimized.

Contributor

samuelstjean commented Feb 4, 2016

You know, it ran fairly quickly as opposed to not at all, so I was really
surprised. Good if it works, but indeed the hcp data is face/background
masked. Should the predict step skip zero valued voxels to not throw of
errors like this or it's left to the unaware at first user?

2016-02-04 16:29 GMT+01:00 Ariel Rokem notifications@github.com:

Good to hear that it works for you.

I am not sure what you are asking about b0/S0. Looks like you have some
voxels with the non diffusion-weighted signal at 0 (outside the brain,
possibly). I am not sure how that would throw off the profiler.


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

@arokem

This comment has been minimized.

Member

arokem commented Feb 4, 2016

"fairly quickly as opposed to not at all" should be our motto :-)

If you don't mask these voxels yourself, it will try to predict them for
you. We could add some magic to avoid that, but that's probably not as
urgent as this fix, so I will defer that to future work (maybe a GSoC on
handling missing data?).

On Thu, Feb 4, 2016 at 8:25 AM, Samuel St-Jean notifications@github.com
wrote:

You know, it ran fairly quickly as opposed to not at all, so I was really
surprised. Good if it works, but indeed the hcp data is face/background
masked. Should the predict step skip zero valued voxels to not throw of
errors like this or it's left to the unaware at first user?

2016-02-04 16:29 GMT+01:00 Ariel Rokem notifications@github.com:

Good to hear that it works for you.

I am not sure what you are asking about b0/S0. Looks like you have some
voxels with the non diffusion-weighted signal at 0 (outside the brain,
possibly). I am not sure how that would throw off the profiler.


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


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

Garyfallidis added a commit that referenced this pull request Feb 9, 2016

Merge pull request #857 from arokem/dti-mem-predict
DTI memory: use the same step in prediction as you use in fitting.

@Garyfallidis Garyfallidis merged commit 463179f into nipy:master Feb 9, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@arokem arokem referenced this pull request Feb 11, 2016

Merged

NF: Free water tensor model #835

RafaelNH added a commit to RafaelNH/dipy that referenced this pull request Mar 5, 2016

NF, Test: Add the input step in the prediction function of fwdti fit …
…class. This will prevent memory issues (as suggested by @arokem I used a strategy similar to nipy#857). Prediction tests were added to access the performance of the new version of the class fwdti fit prediction function

RafaelNH added a commit to RafaelNH/dipy that referenced this pull request Aug 30, 2016

NF, Test: Add the input step in the prediction function of fwdti fit …
…class. This will prevent memory issues (as suggested by @arokem I used a strategy similar to nipy#857). Prediction tests were added to access the performance of the new version of the class fwdti fit prediction function
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment