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

IVIM Workflow #1739

Merged
merged 17 commits into from Mar 4, 2019

Conversation

@ShreyasFadnavis
Copy link
Member

commented Feb 13, 2019

  • Testing
  • Adding remaining parameters
@pep8speaks

This comment has been minimized.

Copy link

commented Feb 13, 2019

Hello @ShreyasFadnavis, Thank you for updating !

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on February 28, 2019 at 19:05 Hours UTC
@codecov-io

This comment has been minimized.

Copy link

commented Feb 13, 2019

Codecov Report

Merging #1739 into master will decrease coverage by 0.01%.
The diff coverage is 71.87%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1739      +/-   ##
==========================================
- Coverage   84.27%   84.25%   -0.02%     
==========================================
  Files         115      115              
  Lines       13683    13714      +31     
  Branches     2158     2165       +7     
==========================================
+ Hits        11531    11555      +24     
  Misses       1649     1649              
- Partials      503      510       +7
Impacted Files Coverage Δ
dipy/sims/voxel.py 90.58% <ø> (ø) ⬆️
dipy/workflows/reconst.py 76.89% <71.87%> (-0.6%) ⬇️
dipy/segment/bundles.py 90.66% <0%> (-0.82%) ⬇️
dipy/reconst/forecast.py 92.22% <0%> (+2.07%) ⬆️
@ShreyasFadnavis

This comment has been minimized.

Copy link
Member Author

commented Feb 14, 2019

@arokem @skoudoro can you take a look and let me know the changes?

@ShreyasFadnavis ShreyasFadnavis changed the title [WIP] IVIM Workflow IVIM Workflow Feb 14, 2019

@skoudoro

This comment has been minimized.

Copy link
Member

commented Feb 15, 2019

Overall, it looks good to me, Thank you @ShreyasFadnavis. Here a couple of comments:

  • Can you rebase your PR?
  • Can you fix all pep8? Look at pep8check comments
  • I think, you should remove small_ivim.nii.gz, small_ivim.bval, small_ivim.bvec. It will be better to generate and save it on your temporary folder, run your workflow with that and then delete them.
@ShreyasFadnavis

This comment has been minimized.

Copy link
Member Author

commented Feb 15, 2019

Overall, it looks good to me, Thank you @ShreyasFadnavis. Here a couple of comments:

  • Can you rebase your PR?
  • Can you fix all pep8? Look at pep8check comments
  • I think, you should remove small_ivim.nii.gz, small_ivim.bval, small_ivim.bvec. It will be better to generate and save it on your temporary folder, run your workflow with that and then delete them.

Thank you @skoudoro ! The reason I added ivim_small is because IVIM needs a special kind of data. I'm not sure of how I can generate them.

I have fixed the pep8 in the last push!

@ShreyasFadnavis ShreyasFadnavis force-pushed the ShreyasFadnavis:ivim_workflow branch from 072fd60 to 0626bbd Feb 15, 2019

if not save_metrics:
save_metrics = ['S0_est', 'f_est', 'D_star_est', 'D_est']

S0_est = ivimfit.model_params[..., 0]

This comment has been minimized.

Copy link
@ShreyasFadnavis

ShreyasFadnavis Feb 16, 2019

Author Member

@arokem I wanted your opinion on this.. Does this look correct to you?
Any advice would be much appreciated!

This comment has been minimized.

Copy link
@arokem

arokem Feb 16, 2019

Member

Yes. Looks right. You can also point to this property if you'd like to be a bit tidier about it.

@arokem
Copy link
Member

left a comment

Nice work! Thanks for pinging me to weigh in, and apologies for the slowness. I had a couple of comments and suggestions.

if not save_metrics:
save_metrics = ['S0_est', 'f_est', 'D_star_est', 'D_est']

S0_est = ivimfit.model_params[..., 0]

This comment has been minimized.

Copy link
@arokem

arokem Feb 16, 2019

Member

Yes. Looks right. You can also point to this property if you'd like to be a bit tidier about it.

S0_est = ivimfit.model_params[..., 0]
f_est = ivimfit.model_params[..., 1]
D_star_est = ivimfit.model_params[..., 2]
D_est = ivimfit.model_params[..., 3]

This comment has been minimized.

Copy link
@arokem

arokem Feb 16, 2019

Member

And same for these: you point to these properties

I'd also advise using the same nomenclature that we used there (e.g., S0_predicted, perfusion_fraction and so forth), unless there is some compelling reason to refer these by different names here.

This comment has been minimized.

Copy link
@ShreyasFadnavis

ShreyasFadnavis Feb 16, 2019

Author Member

Didn't realize, will do! 👍

@@ -777,7 +777,7 @@ def multi_tensor_odf(odf_verts, mevals, angles, fractions):


def single_tensor_rtop(evals=None, tau=1.0 / (4 * np.pi ** 2)):
"""Simulate a Multi-Tensor rtop.
"""Simulate a Single-Tensor rtop.

This comment has been minimized.

Copy link
@arokem

arokem Feb 16, 2019

Member

Good catch!

@@ -997,6 +997,44 @@ def multi_tensor_msd(mf, mevals=None, tau=1 / (4 * np.pi ** 2)):
msd += f * single_tensor_msd(mevals[j], tau=tau)
return msd

def ivim_signal(gtab, S0, f, D_star, D):

This comment has been minimized.

Copy link
@arokem

arokem Feb 16, 2019

Member

This seems to duplicate functionality already implemented in the ivim module. Could we reuse that instead?

This comment has been minimized.

Copy link
@ShreyasFadnavis

ShreyasFadnavis Feb 16, 2019

Author Member

Yes, I have replicated this in sims so that I can use it to test the MIX module which is another PR.
I can take it down from this PR I guess! Let me know :)

This comment has been minimized.

Copy link
@arokem

arokem Feb 18, 2019

Member

Yes. Please remove this and use the implementation in the ivim module as needed.

This comment has been minimized.

Copy link
@ShreyasFadnavis

ShreyasFadnavis Feb 18, 2019

Author Member

Yes. Please remove this and use the implementation in the ivim module as needed.

Will do!

np.savetxt(tmp_bvec_path, bvecs.T)

ivim_flow._force_overwrite = True
# TODO

This comment has been minimized.

Copy link
@arokem

arokem Feb 16, 2019

Member

Is this still a TODO item?

This comment has been minimized.

Copy link
@ShreyasFadnavis

ShreyasFadnavis Feb 16, 2019

Author Member

Yes, @skoudoro suggested this to make Travis happy, due to versioning issues of Python.

This comment has been minimized.

Copy link
@arokem

arokem Feb 16, 2019

Member

So should this code be uncommented?

This comment has been minimized.

Copy link
@ShreyasFadnavis

ShreyasFadnavis Feb 17, 2019

Author Member

I think I will take it down and add it later, once Python 2.7 is discontinued. @skoudoro, sounds good?


S0_est = ivimfit.model_params[..., 0]

This comment has been minimized.

Copy link
@ShreyasFadnavis

ShreyasFadnavis Feb 18, 2019

Author Member

@arokem sorry for the delay, but this is what you wanted me to do right? Let me know if I misunderstood!

save_metrics = ['S0_predicted', 'perfusion_fraction', 'D_star',
'D']

S0_predicted = ivimfit.S0_predicted

This comment has been minimized.

Copy link
@arokem

arokem Feb 18, 2019

Member

Do you really need to refer to these here? You could just refer directly to the ivimfit attributes below as needed.

This comment has been minimized.

Copy link
@ShreyasFadnavis

ShreyasFadnavis Feb 18, 2019

Author Member

Will do as per your suggestion! Thanks :)

dipy/workflows/reconst.py Outdated Show resolved Hide resolved
dipy/workflows/reconst.py Outdated Show resolved Hide resolved
dipy/workflows/reconst.py Outdated Show resolved Hide resolved
dipy/workflows/reconst.py Outdated Show resolved Hide resolved
@@ -997,6 +997,44 @@ def multi_tensor_msd(mf, mevals=None, tau=1 / (4 * np.pi ** 2)):
msd += f * single_tensor_msd(mevals[j], tau=tau)
return msd

def ivim_signal(gtab, S0, f, D_star, D):

This comment has been minimized.

Copy link
@arokem

arokem Feb 18, 2019

Member

Yes. Please remove this and use the implementation in the ivim module as needed.

@ShreyasFadnavis

This comment has been minimized.

Copy link
Member Author

commented Feb 20, 2019

@arokem Done! Thank you for all the help 👍

@skoudoro
Copy link
Member

left a comment

This version looks good! It is close to being in.

I just added a couple of comment but they should easy and quick to fix.

Thank you @ShreyasFadnavis

np.savetxt(tmp_bval_path, bvals)
np.savetxt(tmp_bvec_path, bvecs.T)

ivim_flow._force_overwrite = True

This comment has been minimized.

Copy link
@skoudoro

skoudoro Feb 21, 2019

Member

Your test is great. I think the part between line 70-80 is useless so you can remove it.

This comment has been minimized.

Copy link
@ShreyasFadnavis

ShreyasFadnavis Feb 21, 2019

Author Member

Will do!

return 'ivim'

def run(self, input_files, bvalues_files, bvectors_files, split_b_D=400,
split_b_S0=200, save_metrics=[],

This comment has been minimized.

Copy link
@skoudoro

skoudoro Feb 21, 2019

Member

It would be good if you can add mask_files and b0_threshold=50 as input arguments

This comment has been minimized.

Copy link
@ShreyasFadnavis

ShreyasFadnavis Feb 24, 2019

Author Member

done! 👍

data, affine = load_nifti(dwi)

ivimfit, _ = self.get_fitted_ivim(data, bval, bvec,
b0_threshold=0)

This comment has been minimized.

Copy link
@skoudoro

skoudoro Feb 21, 2019

Member

Why b0_threshold=0?

This comment has been minimized.

Copy link
@ShreyasFadnavis

ShreyasFadnavis Feb 24, 2019

Author Member

Why b0_threshold=0?

Done 👍

@skoudoro

This comment has been minimized.

Copy link
Member

commented Feb 25, 2019

LGTM, Any other comments? I will wait until Wednesday to merge this PR.

@ShreyasFadnavis ShreyasFadnavis force-pushed the ShreyasFadnavis:ivim_workflow branch from fe28317 to 40b1d7e Feb 28, 2019

@ShreyasFadnavis

This comment has been minimized.

Copy link
Member Author

commented Mar 3, 2019

@arokem @skoudoro @Garyfallidis I wanted some help with understanding why 40b1d7e is failing only for Python 3.4?

Any feedback would be helpful!

@arokem

This comment has been minimized.

Copy link
Member

commented Mar 4, 2019

Hmm. I have no idea. Is this even in the code that you wrote?

I'll try to take another look tomorrow in a virtualenv with Python 3.4 installed.

@arokem

This comment has been minimized.

Copy link
Member

commented Mar 4, 2019

Seems like it was intermittent. I reran that job and Travis seems happy now. I'll go ahead and merge.

@arokem arokem merged commit f276b93 into nipy:master Mar 4, 2019

2 of 4 checks passed

codecov/patch CI failed.
Details
codecov/project CI failed.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ShreyasFadnavis

This comment has been minimized.

Copy link
Member Author

commented Mar 4, 2019

Seems like it was intermittent. I reran that job and Travis seems happy now. I'll go ahead and merge.

Thank you so much 👍

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