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

Stats, SNR_in_CC workflow #1663

Merged
merged 5 commits into from Dec 9, 2018

Conversation

Projects
None yet
3 participants
@davhunt
Copy link
Contributor

davhunt commented Nov 9, 2018

Given DWI data (mask, bounding box thresholds optional), computes the signal/noise ratio in the corpus callosum

@skoudoro
Copy link
Member

skoudoro left a comment

Hi @davhunt, Thank you for this workflow!

I need to test it and look more but here my first quick feedback. I will come back to you later.

from nose.tools import assert_true, assert_equal

from dipy.data import get_data
from dipy.workflows.stats2 import SNRinCCFlow

This comment has been minimized.

@skoudoro

skoudoro Nov 9, 2018

Member

I think you should replace stats2 by stats


assert_true(os.path.exists(os.path.join(out_dir, 'product.json')))
assert_true(os.stat(os.path.join(out_dir, 'product.json')).st_size != 0)

This comment has been minimized.

@skoudoro

skoudoro Nov 9, 2018

Member

pep8: Can you remove this empty line?

mask_img = nib.Nifti1Image(mask.astype(np.uint8), vol_img.affine)
mask_path = join(out_dir, 'tmp_mask.nii.gz')
nib.save(mask_img, mask_path)

This comment has been minimized.

@skoudoro

skoudoro Nov 9, 2018

Member

pep8: Can you remove this empty line?

assert_true(os.stat(os.path.join(out_dir, 'product.json')).st_size != 0)


snr_flow.run(*args, mask=mask_path, out_dir=out_dir)

This comment has been minimized.

@skoudoro

skoudoro Nov 9, 2018

Member

before running again snr_flow, you should add snr_flow._force_overwrite = True to make sure you will delete the previous result.

assert_true(os.stat(os.path.join(out_dir,'product.json')).st_size != 0)


snr_flow.run(*args, bbox_threshold=(0.5,1,0,0.15,0,0.15), out_dir=out_dir)

This comment has been minimized.

@skoudoro

skoudoro Nov 9, 2018

Member

before running again snr_flow, you should add snr_flow._force_overwrite = True to make sure you will delete the previous result.

'directions': 'b0' + ' ' + str(SNR_directions[0]) + ' ' + str(SNR_directions[1]) + ' ' + str(SNR_directions[2])
})

with open(os.path.join(out_dir,out_file), 'w') as myfile:

This comment has been minimized.

@skoudoro

skoudoro Nov 9, 2018

Member

pep8: missing whitespace around operator

if direction == 'b0':
SNR = mean_signal[0]/noise_std
logging.info("SNR for the b=0 image is :" + str(SNR))
else :

This comment has been minimized.

@skoudoro

skoudoro Nov 9, 2018

Member

Can you remove the space between else and :

gtab.bvecs[idx] = np.inf
axis_X = np.argmin(np.sum((gtab.bvecs-np.array([1, 0, 0])) **2, axis=-1))
axis_Y = np.argmin(np.sum((gtab.bvecs-np.array([0, 1, 0])) **2, axis=-1))
axis_Z = np.argmin(np.sum((gtab.bvecs-np.array([0, 0, 1])) **2, axis=-1))

This comment has been minimized.

@skoudoro

skoudoro Nov 9, 2018

Member

pep8: missing whitespace around operator. Can you add a space after **

return 'snrincc'

def run(self, data_file, data_bvals, data_bvecs, mask=None, bbox_threshold=(0.6, 1, 0, 0.1, 0, 0.1), out_dir = '', out_file='product.json'):
"""

This comment has been minimized.

@skoudoro

skoudoro Nov 9, 2018

Member

It would be good to have a short description.

mask_noise[..., :mask_noise.shape[-1]//2] = 1
mask_noise = ~mask_noise
mask_noise_img = nib.Nifti1Image(mask_noise.astype(np.uint8), affine)
nib.save(mask_noise_img, 'mask_noise.nii.gz')

This comment has been minimized.

@skoudoro

skoudoro Nov 9, 2018

Member

It seems you save 2 images (cc.nii.gz and mask_noise.nii.gz) so it should be an output.

@skoudoro
Copy link
Member

skoudoro left a comment

Good! Thanks for this update @davhunt, I will come back to you as soon as possible

@skoudoro

This comment has been minimized.

Copy link
Member

skoudoro commented Nov 13, 2018

@pep8speaks Resume now

@skoudoro
Copy link
Member

skoudoro left a comment

Hi @davhunt,

Can you fix all pep8 issue? Thank you!

@davhunt

This comment has been minimized.

Copy link
Contributor

davhunt commented Dec 6, 2018

Hi @skoudoro sorry for the delay-- I think I fixed all of these issues.

@skoudoro
Copy link
Member

skoudoro left a comment

Sorry @davhunt but there is still a bunch of pep8 issues. Below, I just comment a few of them.
I suggest you use a tool like autopep8 to clean this small issues.

pip install autopep8
autopep8 my_py_file --in-place

from dipy.workflows.workflow import Workflow

class SNRinCCFlow(Workflow):

This comment has been minimized.

@skoudoro

skoudoro Dec 6, 2018

Member

pep8: expected 2 blank lines

def get_short_name(cls):
return 'snrincc'

def run(self, data_file, data_bvals, data_bvecs, mask=None, bbox_threshold=(0.6, 1, 0, 0.1, 0, 0.1), out_dir='', out_file='product.json', out_mask_cc='cc.nii.gz', out_mask_noise='mask_noise.nii.gz'):

This comment has been minimized.

@skoudoro

skoudoro Dec 6, 2018

Member

pep8: line too long (203 > 79 characters)

return 'snrincc'

def run(self, data_file, data_bvals, data_bvecs, mask=None, bbox_threshold=(0.6, 1, 0, 0.1, 0, 0.1), out_dir='', out_file='product.json', out_mask_cc='cc.nii.gz', out_mask_noise='mask_noise.nii.gz'):

This comment has been minimized.

@skoudoro

skoudoro Dec 6, 2018

Member

pep8: no space before docstring

mask : string, optional
Path of mask if desired. (default None)
bbox_threshold : string, optional
Threshold for bounding box, values separated with commas for ex. [0.6,1,0,0.1,0,0.1]. (default (0.6, 1, 0, 0.1, 0, 0.1))

This comment has been minimized.

@skoudoro

skoudoro Dec 6, 2018

Member

pep8: line too long (132> 79 characters)

out_mask_cc : string, optional
Name of the CC mask volume to be saved (default 'cc.nii.gz')
out_mask_noise : string, optional
Name of the mask noise volume to be saved (default 'mask_noise.nii.gz')

This comment has been minimized.

@skoudoro

skoudoro Dec 6, 2018

Member

pep8: line too long (83> 79 characters)

b = b.replace("]","")
b = b.replace("(","")
b = b.replace(")","")
b = b.replace(" ","")

This comment has been minimized.

@skoudoro

skoudoro Dec 6, 2018

Member

pep8: missing white space after ','

b = b.replace("(","")
b = b.replace(")","")
b = b.replace(" ","")
b = b.split(",")

This comment has been minimized.

@skoudoro

skoudoro Dec 6, 2018

Member

pep8: missing white space after ','


io_it = self.get_io_iterator()

for data_path, data_bvals_path, data_bvecs_path, out_path, cc_mask_path, mask_noise_path in io_it:

This comment has been minimized.

@skoudoro

skoudoro Dec 6, 2018

Member

pep8: line too long (106> 79 characters)


for data_path, data_bvals_path, data_bvecs_path, out_path, cc_mask_path, mask_noise_path in io_it:
img = nib.load('{0}'.format(data_path))
bvals, bvecs = read_bvals_bvecs('{0}'.format(data_bvals_path), '{0}'.format(data_bvecs_path))

This comment has been minimized.

@skoudoro

skoudoro Dec 6, 2018

Member

pep8: line too long (105> 79 characters)

img = nib.load('{0}'.format(data_path))
bvals, bvecs = read_bvals_bvecs('{0}'.format(data_bvals_path), '{0}'.format(data_bvecs_path))
gtab = gradient_table(bvals, bvecs)

This comment has been minimized.

@skoudoro

skoudoro Dec 6, 2018

Member

pep8: blank line contains white space

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Dec 6, 2018

Codecov Report

❗️ No coverage uploaded for pull request base (master@a600967). Click here to learn what that means.
The diff coverage is 87.5%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1663   +/-   ##
=========================================
  Coverage          ?   90.53%           
=========================================
  Files             ?      234           
  Lines             ?    28322           
  Branches          ?     2993           
=========================================
  Hits              ?    25642           
  Misses            ?     2057           
  Partials          ?      623
Impacted Files Coverage Δ
dipy/workflows/stats.py 83.69% <83.69%> (ø)
dipy/workflows/tests/test_stats.py 95.45% <95.45%> (ø)

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 a600967...0fc3b8f. Read the comment docs.

@davhunt

This comment has been minimized.

Copy link
Contributor

davhunt commented Dec 6, 2018

Ah okay I think now they're all fixed. @skoudoro

@skoudoro

This comment has been minimized.

Copy link
Member

skoudoro commented Dec 7, 2018

Thank you @davhunt.

I will merge this PR at the end of the day if there is no objection

@davhunt

This comment has been minimized.

Copy link
Contributor

davhunt commented Dec 7, 2018

Thank you!

@skoudoro skoudoro merged commit d7c0dc1 into nipy:master Dec 9, 2018

4 checks passed

codecov/patch No report found to compare against
Details
codecov/project No report found to compare against
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment