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

Adds a DKI workflow. #1393

Merged
merged 14 commits into from Feb 10, 2018

Conversation

Projects
4 participants
@arokem
Member

arokem commented Jan 1, 2018

Also, some PEP8 fixes in the workflows/reconst.py file

@arokem arokem force-pushed the arokem:dki-flow branch from 811be10 to e1739f9 Jan 10, 2018

@codecov-io

This comment has been minimized.

codecov-io commented Jan 10, 2018

Codecov Report

Merging #1393 into master will decrease coverage by <.01%.
The diff coverage is 85.52%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1393      +/-   ##
==========================================
- Coverage   86.82%   86.81%   -0.01%     
==========================================
  Files         243      244       +1     
  Lines       30215    30363     +148     
  Branches     3250     3268      +18     
==========================================
+ Hits        26235    26361     +126     
- Misses       3241     3245       +4     
- Partials      739      757      +18
Impacted Files Coverage Δ
dipy/reconst/dki.py 96.82% <ø> (ø) ⬆️
dipy/workflows/reconst.py 82.15% <76.19%> (-1.08%) ⬇️
dipy/workflows/tests/test_reconst_dki.py 97.05% <97.05%> (ø)
dipy/reconst/forecast.py 90.15% <0%> (-2.08%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cb33390...d9bc7af. Read the comment docs.

@arokem

This comment has been minimized.

Member

arokem commented Jan 10, 2018

Rebased on master after #1400.

Anyone want to review this?

@skoudoro

Thanks for this workflow @arokem. I just made a couple of cosmetic changes but it looks good!

Can you add dipy_dki executable in bin folder?

if mask is None:
mask = None
else:

This comment has been minimized.

@skoudoro

skoudoro Jan 11, 2018

Member

Is it really necessary to do the if else ?

Why not if mask is not None ?

(default [] (all))
out_dir : string, optional
Output directory (default input file directory)
out_tensor : string, optional

This comment has been minimized.

@skoudoro

skoudoro Jan 11, 2018

Member

Unexpected parameter out_tensor in docstring. The function run has out_dt_tensor and out_dk_tensor

if 'mk' in save_metrics:
mk_img = nib.Nifti1Image(dkfit.mk().astype(np.float32),
affine)

This comment has been minimized.

@skoudoro

skoudoro Jan 11, 2018

Member

PEP8: continuation of line

if 'ak' in save_metrics:
ak_img = nib.Nifti1Image(dkfit.ak().astype(np.float32),
affine)

This comment has been minimized.

@skoudoro

skoudoro Jan 11, 2018

Member

PEP8: continuation of line

if 'rk' in save_metrics:
rk_img = nib.Nifti1Image(dkfit.rk().astype(np.float32),
affine)

This comment has been minimized.

@skoudoro

skoudoro Jan 11, 2018

Member

PEP8: continuation of line

if __name__ == '__main__':

This comment has been minimized.

@skoudoro

skoudoro Jan 11, 2018

Member

PEP8: Too many blank line

@skoudoro skoudoro added this to Needs a review in Dipy 0.14.0 Jan 17, 2018

@arokem arokem force-pushed the arokem:dki-flow branch from e1739f9 to 7b325b2 Jan 31, 2018

@arokem

This comment has been minimized.

Member

arokem commented Jan 31, 2018

I think that addressed all your comments, @skoudoro ?

@skoudoro

Thanks @arokem. I just need to test it one last time but it looks ready to go!

tensor_path = dki_flow.last_generated_outputs['out_dt_tensor']
tensor_data = nib.load(tensor_path)
assert_true(tensor_data.shape[-1] == 6)

This comment has been minimized.

@skoudoro

skoudoro Jan 31, 2018

Member

I think you prefer assert_equal than assert_true like you said on #1387 ;-)

@skoudoro

Hi @arokem, can you rename dipy_reconst_dki to dipy_fit_dki as proposed on PR #1358.

Thank you

def get_short_name(cls):
return 'dki'
def run(self, input_files, bvalues, bvectors, mask_files, b0_threshold=0.0,

This comment has been minimized.

@skoudoro

skoudoro Feb 7, 2018

Member

After playing with this workflow, I wonder if mask_files can be optional.

It was a constraint to generate a mask before running this model when I just want to test it on the whole image (or I was lazy :-))

This comment has been minimized.

@arokem

arokem Feb 7, 2018

Member

I think that setting default to None (i.e., use all voxels) would be fine.

But it needs to be consistently applied across all the workflows (see for example the dipy_dti_fit flow, see: https://github.com/nipy/dipy/blob/master/dipy/workflows/reconst.py#L30).

This comment has been minimized.

@skoudoro

skoudoro Feb 8, 2018

Member

Agree with consistency, so no need to change it. Apparently, we want to give good habits to users

@arokem arokem force-pushed the arokem:dki-flow branch from 2c184e9 to f0c42d3 Feb 7, 2018

@arokem

This comment has been minimized.

Member

arokem commented Feb 7, 2018

Renamed the script and rebased!

@arokem

This comment has been minimized.

Member

arokem commented Feb 8, 2018

@skoudoro

This comment has been minimized.

Member

skoudoro commented Feb 8, 2018

No, everything good, I just tested it again on DKI dataset and the workflow works well.

Can you just rebase? Then I can merge it.

Thank you @arokem

@arokem arokem force-pushed the arokem:dki-flow branch from f0c42d3 to a809433 Feb 8, 2018

if mask is None:
mask = None
else:
if mask is not None:

This comment has been minimized.

@arokem

arokem Feb 8, 2018

Member

OK - I am not sure why this is here. @Garyfallidis : under what circumstances would the mask be a None? It's a required input into the workflow?

This comment has been minimized.

@Garyfallidis

Garyfallidis Feb 9, 2018

Member

When you run the Flow from a Python script it is easy to put a None. But not easy from the command line. It may be good to enforce the same behavior for consistency.

This comment has been minimized.

@arokem

arokem Feb 9, 2018

Member

That makes sense! What do you think about defaulting to None, so that if users don't pass a mask, we use this option? We can do that on a separate PR, to make sure to do this uniformly across the different flows.

This comment has been minimized.

@arokem

arokem Feb 9, 2018

Member

All that's to say: I think this PR is ready to go! @skoudoro : would you mind giving this another look?

This comment has been minimized.

@skoudoro

skoudoro Feb 9, 2018

Member

I think it will be good to just raise an exception if there is no mask. In that way,

  1. we keep the consistency with the command line
  2. we give good habits to the user by forcing him to generate a mask

But I agree with @arokem, we can do that in a separate PR

@arokem

This comment has been minimized.

Member

arokem commented Feb 8, 2018

Rebased!

Name of the axial kurtosis to be saved (default: 'ak.nii.gz')
out_rk : string, optional
Name of the radial kurtosis to be saved (default: 'rk.nii.gz')
"""

This comment has been minimized.

@skoudoro

skoudoro Feb 9, 2018

Member

Can you add references/citations?

@skoudoro

This comment has been minimized.

Member

skoudoro commented Feb 9, 2018

LGTM, I let you fix my last comment, and I can merge it today

@arokem

This comment has been minimized.

Member

arokem commented Feb 9, 2018

@@ -581,6 +581,18 @@ def run(self, input_files, bvalues, bvectors, mask_files, b0_threshold=0.0,
Name of the axial kurtosis to be saved (default: 'ak.nii.gz')
out_rk : string, optional
Name of the radial kurtosis to be saved (default: 'rk.nii.gz')
Notes

This comment has been minimized.

@skoudoro

skoudoro Feb 9, 2018

Member

References instead of Notes

@arokem

This comment has been minimized.

Member

arokem commented Feb 9, 2018

@skoudoro

This comment has been minimized.

Member

skoudoro commented Feb 9, 2018

I just checked on numpy docstring standard and we do not diverge from it. References are not in Notes section.
I do not know if it is something new or not

@arokem

This comment has been minimized.

Member

arokem commented Feb 9, 2018

@arokem arokem force-pushed the arokem:dki-flow branch from 62ff3fb to d9bc7af Feb 9, 2018

@skoudoro

This comment has been minimized.

Member

skoudoro commented Feb 9, 2018

Thank you for completing this information

@skoudoro

This comment has been minimized.

Member

skoudoro commented Feb 10, 2018

Thanks for this workflow @arokem ! merging!

@skoudoro skoudoro merged commit 177bdde into nipy:master Feb 10, 2018

1 of 3 checks passed

codecov/patch 85.52% of diff hit (target 86.82%)
Details
codecov/project 86.81% (-0.01%) compared to cb33390
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

Dipy 0.14.0 automation moved this from Needs a review to Done Feb 10, 2018

ShreyasFadnavis pushed a commit to ShreyasFadnavis/dipy that referenced this pull request Sep 20, 2018

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