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

Workflow for visualizing the quality of the registered data with DIPY #1595

Closed
wants to merge 44 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@parichit
Copy link
Contributor

parichit commented Jul 25, 2018

This branch is created from the older 'affine_visual' branch and only focuses on integrating the visualization support for the registered images.

These changes will form the part of the visual quality assessment in the DIPY workflows and will complement the quantitative assessment from the affine_registration branch.

Parichit Sharma and others added some commits Jun 18, 2018

Parichit Sharma
Added the image registration workflow to the align.py and test_align.py
Since the image registration forms the part of alignment so the image registration workflow is added to the align class.

1) Added the ImageRegistrationWorkflow Class to the align.py file.
Parichit Sharma
Added a command line wrapper for the Image Registration workflow.
1) Created the separate command line script for the Image Registration workflow.
Parichit Sharma
Workflow for applying the transform on a set of images.
1)Added the function to load the saved affine matrix that needs to be applied.

2) Added the import for scipy.ndimage in the test_parzenhist.py file.

3) Added the apply_transform workflow to apply the transform.
Parichit Sharma
Added the reporting of quality metric values.
1) The distance between the images after registration is reported.
2) The metric of optiaml parameters is reported during the workflow.
3) The quality assurance metric can be saved optionally by the user.

4) Tested the workflows on simulated (coronal slice dataset) and real datasets (stanford hardi).
Parichit Sharma
Added a function for producing aligned images in align.py.
1) An image containing the slices from static and moving images with different colur channel
is produced.
Parichit Sharma
Testing with different visualisation outputs.
1) Created a function for playing the png of slices after registration.
2) Testing the conversion between the VTK and numpy format.
Parichit Sharma
Modified the code to create animation in correct orientation.
1) Renamed the functions properly.
2) Orientation is corrected by extracting the individual slices slices.
3) Animation is done using the Matplotlib's native functionality.

Future Work
1) Try to create the animation for different views (saggittal and coronal)
2) Try to create a grid of animated plots showing the registration progress in different views.
Parichit Sharma
Updated the functions for producing the mosaic and animations with ge…
…neral parameters.

1) Removed the hard coded values (from earlier testing).
2) Made the code more generalized to accept the parameters from the user.
3) Slightly, modified the overlay_slices routine (from regtools.py) to return the
apprpriate slice to be used in the animation.
Parichit Sharma
Commit for the stable version of the visualisation code
List of changes

1) An additional function(s) are integrated with the affine registration worklfow.
2) The creation of mosaic is separate from the creation of the GIF animation.
3) The animation creation is currently experimental with additional support for the
animating the slices in different planes.
4) The workflow is made generic to handle user requests by providing command line parameters.
Parichit Sharma
Modifications:
1) Copied changes from the files (local files on the system, not part of the branch) to the align.py file.
2) Updated the options for selecting a specific type of slice from indixes to strings (for user=friendliness).
3) Added another function (still under experimentation) to use the renderer for creating the animation.
4) Checked the correct orientation when creating the animation via the renderer.
5) Made minor change to the native window.py file to renturn the array if filename is None. This saves
extra processing in creating and handling the VTK writer object.
Parichit Sharma
Workflow for visualizing the quality of registered image.
Note:
1) This workflow has been created by moving changes from an older branch ('affine_visual') because
there was the need to isolate all the visualization related changes to an individual workflow. Eventually,
these changes will be removed from the affine_visual branch because they will conflict with the affine_registration
workflow.

2) Some part of this code is being directly bought in from local files (which are not part of the repo) because
the code was being repeteadly for debugging and for improving the quality of the images.

First release of the workflow

1) Adding the function to create the mosaic from the slices. The function is different from the previous
commits (in the older branch 'affine_visual') because it also needs to read the afine matrix.

2) Made a minor addition to the image.py file to load the affine matrix for transforming the moved image to the
static image reference system.

3) Added a static method for checking the dimensions of the static and the moved image.
@pep8speaks

This comment has been minimized.

Copy link

pep8speaks commented Jul 25, 2018

Hello @parichit, Thank you for updating !

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

Comment last updated on July 27, 2018 at 20:11 Hours UTC

Parichit Sharma added some commits Jul 25, 2018

Parichit Sharma
1) Fixed the PEP8 issues.
2) Removed the align.py file from the commits.
Parichit Sharma
Added a a function to create the GIF animation.
List of changes

1) Added the command line flag and its documentation in the run method to support the GIF creation.
2) Added the conditional check to call the appropriate functions.
3) Updated the process_image_data() function to selectively calculate the range of values to be used for visualisation.
Parichit Sharma
Added one more experimental function 'animate_overlaid_with_renderer'
1) This function provides the ability to control the dimensions of the created GIF animation.
2) It also provides the feature to correct the image orientation by applying the correct
affine transformation.
Parichit Sharma
@Garyfallidis

This comment has been minimized.

Copy link
Member

Garyfallidis commented Jul 25, 2018

This should be merged after #1581 is merged.

Parichit Sharma added some commits Jul 25, 2018

Parichit Sharma
Rectified the color range problem in the GIF
1) Updated the code by solving the issue due to higher range of color while creating the GIF.
2) Scaled down the range of colors by manually intepolating the color channels in the image.
3) Added supplementary function to interpolate the range of the color values.
Parichit Sharma
Parichit Sharma
1) Removed the animate_slices() function because the animate_slices_w…
…ith_renderer() is working

correctly now. Renamed animate_slices_with_renderer() as animate_slices() because 'renderer'
be the default choice now for creating the GIF animation.

2) Updated the command line help for the options.
@parichit

This comment has been minimized.

Copy link
Contributor

parichit commented Jul 25, 2018

This PR must only be merged after PR 1581 and PR 1598. The exact order should be,

PR 1581 -> PR 1598 -> PR 1595.

This is so because the PR 1595 was being developed individually and later it was decided to make the result visualization for quality assurance into a separate workflow. So, the code was removed from the align.py (affine_registration) workflow and put into a separate workflow for quality assessment.

Parichit Sharma added some commits Jul 26, 2018

@nilgoyette
Copy link
Contributor

nilgoyette left a comment

I didn't finish the review because some of my comments imply changing a lot of code, so I prefer letting you work on it more.

Also, I won't answer much (or at all) because I'm leaving for vacation.

cols = num_slices // i
break

return rows, cols

This comment has been minimized.

@nilgoyette

nilgoyette Jul 26, 2018

Contributor

This whole function can probably be simple mathematic. The first loop could be something like

num_slices = int((num_slices / 5).ceil())

As for the second loop, I'm not even sure what it does. If you absolutely need a loop, please explain what you do with a comment.

This comment has been minimized.

@parichit

parichit Jul 26, 2018

Contributor

yeah, I have to find a way to improve this function. The way it is right now it works well for most inputs but it needs more improvement. Not just the conciseness as you suggested but the mathematics itself can be optimized much. I am simply trying to factor a number into its 2 factors so that the number of rows and columns that I get back can be used in the create_mosaic function.

So, if there are 74 slices (for example in an image, I increase it to 75 to make it a multiple of 5). It can be broken down into either 25 *3 or 5 * 15 or 75 * 1 etc. but you will agree that having 75 rows and 1 column or 1 row and 75 column is not the best way to show the slices side by side. Similarly, 25 rows and 3 columns is also not a good option. But, I can use 5 * 15 means 5 rows and 15 columns for visualizing all the slices side by side.

Arriving at this combination is made difficult by the fact that I can have different number of slices in different inputs so what is the most optimal number of rows and columns to create the mosaic?

I need to come up either with a better algorithm or a clever trick to achieve this.

But many thanks for sharing your concerns with me. I really appreciate your help.

def get_row_cols(self, num_slices):

"""
Experimetal helper function to get the number

This comment has been minimized.

@nilgoyette

nilgoyette Jul 26, 2018

Contributor

So metal ;)

"""

overlay, value_range = self.process_image_data(static_img,
moved_img)

This comment has been minimized.

@nilgoyette

nilgoyette Jul 26, 2018

Contributor

You sometime have random \n where the code would actually fit on a 79 characters line. Here. The def create_mosaic line above. Probably more.

This comment has been minimized.

@parichit

parichit Jul 26, 2018

Contributor

Yup, It fits in 67 characters, fixed it. Thanks.

renderer.projection('parallel')

cnt = 0
X, Y, num_slices = slice_actor.shape[:3]

This comment has been minimized.

@nilgoyette

nilgoyette Jul 26, 2018

Contributor

I thought PEP8 didn't like upper case names. Isn't there a warning?

This comment has been minimized.

@parichit

parichit Jul 26, 2018

Contributor

Yes, PEP8 don't like capital letter names but I kept it that way to remind me that these are the image dimensions. I was experimenting so there were small case x,y and z too. Reverted to the small case now.

slice_type = 1
elif sli_type == 'axial':
num_slices = z
slice_type = 2

This comment has been minimized.

@nilgoyette

nilgoyette Jul 26, 2018

Contributor

It's sagittal. Also, what happens when it's something else than those three names? Ok, you write None (default). but it looks like this function is doing nothing on None.

This comment has been minimized.

@parichit

parichit Jul 26, 2018

Contributor

Yup, it is 'sagittal', corrected the typo. Also, earlier I was checking the 'None' inside the functions and returning immediately if that was true. Later I decided to check for this in the run method but forgot to remove the 'None' from the function documentation. Good point, though. Thanks.

"but some information was lost.\nPlease check your gif and "
"convert to uint8 beforehand if the gif looks wrong."
)
warnings.warn(message)

This comment has been minimized.

@nilgoyette

nilgoyette Jul 26, 2018

Contributor

Maybe this is more a question of style, so don't feel forced to change it. I see no advantage in creating a message from a string, then calling a warning with it. Why don't you simply write it in the argument? It takes less place. Idem for this pattern at other places.

warnings.warn(
    "\nYour image was cast to a `uint8` (`<img>.astype(uint8)`), "
    "but some information was lost.\nPlease check your gif and "
    "convert to uint8 beforehand if the gif looks wrong.")

Also, it's the first time I see a warning starting by \n. Is this normal in DiPy?

if four_d or not isinstance(dataset, numpy.ndarray):
return _make_animated_gif(d, delay_time=delay_time)
else:
return _make_gif(d)

This comment has been minimized.

@nilgoyette

nilgoyette Jul 26, 2018

Contributor

Again, maybe a question of style, but a else after a return is useless.

import numpy as np
import nibabel as nib
from dipy.viz.regtools import overlay_slices
from dipy.viz import (window, actor)

This comment has been minimized.

@nilgoyette

nilgoyette Jul 26, 2018

Contributor

Useless ()

This comment has been minimized.

@parichit

parichit Jul 26, 2018

Contributor

yes, thanks.

break

return rows, cols

This comment has been minimized.

@nilgoyette

nilgoyette Jul 26, 2018

Contributor

Hey, I already commented on this function in another file! We rarely have to copy code. DRY. In fact, I recognize some of the code in this file. What's happening? Can't you use common functions?

This comment has been minimized.

@parichit

parichit Jul 26, 2018

Contributor

So here is the thing: This branch was originally part of the affine_registration PR 1581 and so you will remember that you suggested a good number of improvements to that PR. I did all those changes to that PR and I was hoping that it will be merged. Meanwhile, I continued working on this branch thinking that when PR 1581 will be merged, I can just rebase and use all the latest changes here.

But, this branch reached its's completion and the PR 1581 is still not merged so a large number of changes that you said, for example, using assert and not npt.assert_equal or not using () while importing or sticking to just one standard for documentation in the functions have already been done in PR 1581. nevertheless, i manually copied all the changes from my local repo for PR 1581 and put them here so that they align with all the improvements that I made in PR 1581.

Sorry for the inconvenience and thanks very much for the useful comments :).

"""

if sli_type == 3:

This comment has been minimized.

@nilgoyette

nilgoyette Jul 26, 2018

Contributor

Why would slice_type : str (optional) be equal to 3?

Parichit Sharma added some commits Jul 26, 2018

Parichit Sharma
Updated the align.py file in this branch by manually copying the late…
…st changes from the affine_registration branch.
Parichit Sharma
Parichit Sharma
List of changes:
1) Fit the error message from the check_dimensions function to fit in 2 lines for readability.
2) Renamed the animate_overlap_with_renderer to animate_overlap since there is only this fucntion now.
3) Renamed 'sli_type' to 'slice_type' in the animate_overlap() function.
4) Removed the 'None' from the function documentation in the animate_overlap() because the None condition is now
being checked in the run method of the workflow.
5) Renmaed the upper case X, Y and Z to small case x, y and z in the create_mosaic function.
6) Fit the create_mosaic function in one line.
Parichit Sharma
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jul 26, 2018

Codecov Report

Merging #1595 into master will decrease coverage by 0.72%.
The diff coverage is 27.67%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1595      +/-   ##
==========================================
- Coverage   87.32%   86.59%   -0.73%     
==========================================
  Files         246      248       +2     
  Lines       31806    32258     +452     
  Branches     3450     3513      +63     
==========================================
+ Hits        27775    27935     +160     
- Misses       3210     3497     +287     
- Partials      821      826       +5
Impacted Files Coverage Δ
dipy/viz/regtools.py 32.5% <0%> (-0.56%) ⬇️
dipy/align/imaffine.py 91.84% <100%> (+0.04%) ⬆️
dipy/workflows/flow_runner.py 97.43% <100%> (ø) ⬆️
dipy/workflows/align.py 77.89% <100%> (-15.86%) ⬇️
dipy/workflows/vis_registeration.py 15.92% <15.92%> (ø)
dipy/workflows/core.py 17.22% <17.22%> (ø)
dipy/io/image.py 81.48% <50%> (-18.52%) ⬇️
dipy/workflows/tests/test_align.py 94.91% <94.87%> (+4%) ⬆️
dipy/core/graph.py 73.8% <0%> (-1.2%) ⬇️
... 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 6e1da86...e5f6653. Read the comment docs.

Parichit Sharma and others added some commits Jul 26, 2018

Parichit Sharma
Merge branch 'nipy-dipy-master' into visualisation_modules
Merging the latest changes from the DIPY master.
@skoudoro

This comment has been minimized.

Copy link
Member

skoudoro commented Aug 21, 2018

closing in favor of #1606

@skoudoro skoudoro closed this Aug 21, 2018

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