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: Ivim implementation #1055

Closed
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@sahmed95
Contributor

sahmed95 commented May 21, 2016

I will be working on this over the summer as a GSoC project mentored by @arokem, @RafaelNH and Eric Peterson. Inside the folder ivim_dev, I have a working code built on Eric's initial implementation (https://github.com/etpeterson) which has been modified to follow the dki code. A Jupyter notebook demonstrates the earlier work using simulated data. It closely follows the dki code.

We will go part by part with necessary tests at each stage. ivim.py is the main file and in my opinion reviewing ivim_dev shouldn't be a priority for now as most of it will be changed

The model fits the equation S = S0(f*e^(-bD_star) + (1-f)e^(-bD)) which gives a measure of the diffusion and perfusion parameters. You can find the complete proposal here

Shahnawaz Ahmed

@arokem arokem added the gsoc2016 label May 21, 2016

@sahmed95

This comment has been minimized.

Contributor

sahmed95 commented May 21, 2016

Ah, I think I should have broken down the commits into smaller parts !

@sahmed95 sahmed95 referenced this pull request May 21, 2016

Open

it's finally here #1

@arokem

This comment has been minimized.

Member

arokem commented May 21, 2016

Yes. This is probably too large to be merged as one piece. We usually prefer PRs with less LOC: there's empirical evidence that reviewing more than 200 LOC becomes unwieldy. We've seen that here. For example, #740 is languishing among other reasons because it's so large. But let's leave this here for reference, so that we can see what you are thinking about. We can break it apart for review, after you explain all the moving pieces to us (on our hang out this Monday).

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented May 21, 2016

Hello @sahmed95. Thank you for this PR. This is a reconstruction model so the main module (ivim.py) should be under dipy/reconst also we need to break this as Ariel says in many PRs and decide which tutorials will be available and which not. I see also that you use ipython notebooks which is something which is the first time to save directly in the codebase. So, we need to do this with care. I saw you have another related PR. Thanks again and best of luck with GSoC!

@etpeterson

This comment has been minimized.

Contributor

etpeterson commented May 22, 2016

Hi @sahmed95, good to see this. I'll take a look and I'm looking forward to talking it though on Monday.

@sahmed95

This comment has been minimized.

Contributor

sahmed95 commented May 22, 2016

Ok, I am getting the hang of it now. I will keep pushing smaller commits. In "clean slate" I removed all previous code. This can be ignored and reviewing can be started from the next commit.

@Garyfallidis I was using the Ipython notebooks for my own reference and for trying out code. I got rid of it and will only upload the final codes from now on. As you suggested, I have moved ivim.py into reconst. Next up is writing tests. Will keep updating in smaller commits.

Thanks.

Shahnawaz Ahmed

@sahmed95 sahmed95 force-pushed the sahmed95:ivim branch from f9ecda1 to 61fd3fd May 22, 2016

@etpeterson

This comment has been minimized.

Contributor

etpeterson commented May 23, 2016

Here's an IVIM dataset: https://figshare.com/s/4733b6fc92d4977c2ee1

Let me know if you can't get to it for some reason, I'm still new to figshare so I may not have gotten everything right.

@RafaelNH

This comment has been minimized.

Contributor

RafaelNH commented May 30, 2016

Hi @sahmed95! How are things going? Any advances on the simulations/IVIM-fitting (based on Bihan et al., 1988)

@sahmed95

This comment has been minimized.

Contributor

sahmed95 commented May 30, 2016

Hi @RafaelNH, I forgot to post here. Please find the initial code and simulations here - #1058

I will work on implementing leastsq and carry out some more simulations today.

@RafaelNH

This comment has been minimized.

Contributor

RafaelNH commented May 30, 2016

Cool @sahmed95! I will start reviewing what you did =)

@arokem

This comment has been minimized.

Member

arokem commented May 31, 2016

Closing in favor of #1058

@arokem arokem closed this May 31, 2016

@sahmed95 sahmed95 deleted the sahmed95:ivim branch May 31, 2016

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