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

Cleaned PR for Visualization Modules to Assess the quality of Registration Qualitatively. #1606

Closed
wants to merge 33 commits into from

Conversation

@parichit
Copy link
Contributor

commented Jul 31, 2018

This work was done as part of the Google Summer of Code (GSOC-2018) DIPY project titled 'DIPY Workflow(s) and Quality Assurance'

This work pertains to developing a workflow to visualize the quality of the registered images by creating a mosaic or the animation or both depending on the requirement. The plane to be visualized (Axial, Coronal or sagittal) can be easily controlled through the command line parameters.

This is a cleaned PR for the visualization modules developed to support the Image Registration Workflow(s). The PR1595 must not be merge now. Instead, this should be merged after making the changes in the core.py file for the GIF color range issues. The exact order of merging the PR's should be

PR1604 (affine registration workflow) -> PR1605 (apply transformation workflow) - > PR1606 (visualisation)

The link to the older PR (with all the conversations, commit history, improvements based on community feedback can be seen below):
#1595

@pep8speaks

This comment has been minimized.

Copy link

commented Jul 31, 2018

Hello @parichit, Thank you for updating !

Line 426:31: W292 no newline at end of file

Comment last updated on August 13, 2018 at 21:22 Hours UTC

@skoudoro skoudoro added the gsoc2018 label Aug 1, 2018

@parichit parichit force-pushed the parichit:visualisation_modules_clean branch from 19c44ce to b37d97d Aug 7, 2018

Decreased the decimal precision to 1 decimal places (in checking the …
…value of distance metric) to validate the tests by travis.

@parichit parichit force-pushed the parichit:visualisation_modules_clean branch from b37d97d to e06854a Aug 13, 2018

1) Updated the parameter documentation for optimal parameters and sim…
…ilarity metric in the optimize() function of the imaffine.py file.

2) Changed the parameter documentation in the run() method of the alignpy file for improved view on the command line.

@parichit parichit force-pushed the parichit:visualisation_modules_clean branch from edcdfec to 742f68b Aug 13, 2018

@codecov-io

This comment has been minimized.

Copy link

commented Aug 13, 2018

Codecov Report

Merging #1606 into master will decrease coverage by 0.63%.
The diff coverage is 42.39%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1606      +/-   ##
==========================================
- Coverage   87.34%   86.71%   -0.64%     
==========================================
  Files         246      249       +3     
  Lines       31811    32344     +533     
  Branches     3451     3519      +68     
==========================================
+ Hits        27785    28046     +261     
- Misses       3204     3472     +268     
- Partials      822      826       +4
Impacted Files Coverage Δ
dipy/viz/regtools.py 32.5% <0%> (-0.56%) ⬇️
dipy/io/image.py 100% <100%> (ø) ⬆️
dipy/align/imaffine.py 91.84% <100%> (+0.04%) ⬆️
dipy/workflows/align.py 92.63% <100%> (-1.12%) ⬇️
dipy/workflows/core.py 17.22% <17.22%> (ø)
dipy/workflows/vis_registration.py 24.36% <24.36%> (ø)
dipy/workflows/tests/test_vis.py 93.1% <93.1%> (ø)
dipy/workflows/tests/test_align.py 98.09% <97.77%> (+7.18%) ⬆️
... and 3 more

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 5a6aa5a...73fc149. Read the comment docs.

parichit added 9 commits Aug 2, 2018
1) Renamed the command line wrapper for the syn_registration.
2) Added a utility funciton in the vis_register.py to check the slice type and raise the error.
1) Renamed the ApplyTransformFlow to ApplyAffineFlow to distinguish i…
…t from the Syn_Registration flow.

2) Made the changes in the command line wrapper to use the changed name for the workflow.
Changed the name of the function to test_apply_affine_transform() and…
… decreased the decimal precision to 1 decimal place for validating the travis tests.
@Borda
Copy link
Contributor

left a comment

there is missing documentation for several functions and some are incomplete
overall I feel that the PR is too long and complex 8-)

@@ -1074,6 +1083,8 @@ def optimize(self, static, moving, transform, params0,
self.params0 = self.transform.get_identity_parameters()

affine_map.set_affine(self.starting_affine)
if ret_metric:
return affine_map, opt.xopt, opt.fopt

This comment has been minimized.

Copy link
@Borda

Borda May 16, 2019

Contributor

I do not think that it is a good idea to optionally return a different number of results...



def load_nifti(fname, return_img=False, return_voxsize=False,
return_coords=False):
img = nib.load(fname)
data = img.get_data()
vox_size = img.header.get_zooms()[:3]

This comment has been minimized.

Copy link
@Borda

Borda May 16, 2019

Contributor

why new line?

---------
fname : str
The file containing the saved affine matrix.
"""

This comment has been minimized.

Copy link
@Borda

Borda May 16, 2019

Contributor

include return description



def save_quality_assur_metric(fname, xopt, fopt):
"""

This comment has been minimized.

Copy link
@Borda

Borda May 16, 2019

Contributor

you should say that the output is text and back parsable because you add fopt at the end of the file...

@@ -384,6 +389,9 @@ def overlay_slices(L, R, slice_index=None, slice_type=1, ltitle='Left',
colorImage[..., 0] = ll * (ll > ll[0, 0])
colorImage[..., 1] = rr * (rr > rr[0, 0])

if ret_slice:

This comment has been minimized.

Copy link
@Borda

Borda May 16, 2019

Contributor

I would rather use this function to return slice always and define a new function with a slice image as input and create a figure with an overlay

assert os.path.exists(affine_mat_file)
return True

test_com()

This comment has been minimized.

Copy link
@Borda

Borda May 16, 2019

Contributor

what is the reason for defining all this function when they are used only once

"""
rows, cols = 0, 0

while True:

This comment has been minimized.

Copy link
@Borda

Borda May 16, 2019

Contributor

compute it directly, no loops are needed
num_slices += 5 - (num_slices % 5) if (num_slices % 5) > 0 else 0

for i in range(5, num_slices):

if num_slices % i == 0:
rows = i

This comment has been minimized.

Copy link
@Borda

Borda May 16, 2019

Contributor

compute it directly, no loops are needed

renderer.reset_camera()
renderer.zoom(1.6)
cnt += 1
if cnt > num_slices:

This comment has been minimized.

Copy link
@Borda

Borda May 16, 2019

Contributor

how/why this can happen?


num_slices = 0

if slice_type == 'sagittal':

This comment has been minimized.

Copy link
@Borda

Borda May 16, 2019

Contributor

I would use a dictionary

slice_dict = {
    'sagittal': (x, 0),
    'coronal': (y, 1),
    'axial': (z, 2)
}
num_slices, slice_type = slice_dict[slice_type]
@arokem

This comment has been minimized.

Copy link
Member

commented May 22, 2019

Is this PR still in progress? Looks like not much work has happened here recently, so I am going to close this. Feel free to reopen if you'd like to pick this up.

@arokem arokem closed this May 22, 2019

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