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

Image registration Workflow with quality metrices #1581

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@parichit
Copy link
Contributor

parichit commented Jul 1, 2018

  1. Opening the PR for the image registration workflow.
  2. The tests have been written for both correct and incorrect input.
  3. The older branches have been removed and the code is included in the align.py and test_align.py files for code consistency.
  4. Also, added the reporting of quality metrics such as the distance between the registered images and the metric of optimal parameters (to assist in quantitative quality assessment).
@pep8speaks

This comment has been minimized.

Copy link

pep8speaks commented Jul 1, 2018

Hello @parichit, Thank you for updating !

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

Comment last updated on July 30, 2018 at 19:27 Hours UTC

@skoudoro skoudoro added the gsoc2018 label Jul 2, 2018

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jul 3, 2018

Codecov Report

Merging #1581 into master will increase coverage by <.01%.
The diff coverage is 88.88%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1581      +/-   ##
==========================================
+ Coverage   87.34%   87.34%   +<.01%     
==========================================
  Files         246      246              
  Lines       31811    32048     +237     
  Branches     3451     3482      +31     
==========================================
+ Hits        27785    27993     +208     
- Misses       3204     3226      +22     
- Partials      822      829       +7
Impacted Files Coverage Δ
dipy/workflows/flow_runner.py 97.29% <ø> (-0.14%) ⬇️
dipy/align/imaffine.py 91.84% <100%> (+0.04%) ⬆️
dipy/workflows/align.py 89.53% <100%> (-4.22%) ⬇️
dipy/io/image.py 84% <50%> (-16%) ⬇️
dipy/workflows/tests/test_align.py 94.91% <94.87%> (+4%) ⬆️
dipy/reconst/mapmri.py 90.28% <0%> (-0.65%) ⬇️
dipy/reconst/tests/test_dsi_metrics.py 91.66% <0%> (-0.34%) ⬇️
dipy/tracking/streamline.py 67.68% <0%> (-0.28%) ⬇️
... and 13 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...9c6555b. Read the comment docs.


from dipy.align.tests.test_parzenhist import setup_random_transform
from dipy.align.transforms import (Transform,
regtransforms)

This comment has been minimized.

@nilgoyette

nilgoyette Jul 5, 2018

Contributor

One line

This comment has been minimized.

@parichit

parichit Jul 6, 2018

Contributor

@nilgoyette: Done, Thanks for pointing this out.

@@ -20,12 +31,127 @@ def test_reslice():
out_path = reslice_flow.last_generated_outputs['out_resliced']
out_img = nib.load(out_path)
resliced = out_img.get_data()


npt.assert_equal(resliced.shape[0] > volume.shape[0], True)
npt.assert_equal(resliced.shape[1] > volume.shape[1], True)
npt.assert_equal(resliced.shape[2] > volume.shape[2], True)
npt.assert_equal(resliced.shape[-1], volume.shape[-1])

This comment has been minimized.

@nilgoyette

nilgoyette Jul 5, 2018

Contributor

You have 2 kinds of use of assert_equal here. One that I consider legitimate (comparing to something else) and one where you compare to True. An assert_equal(wathever, True) is an assert.

There's no assert in numpy.testing, probably because python already offers one. It's a statement though, not a real function. Maybe DiPy is against?

This comment has been minimized.

@nilgoyette

nilgoyette Jul 10, 2018

Contributor

@skoudoro and others, do you have an opinion on the use of assert? I don't like statements much, but I like assert_equal(wathever, True) even less.

This comment has been minimized.

@skoudoro

skoudoro Jul 10, 2018

Member

I am not against assert and I think we should use it here

This comment has been minimized.

@parichit

parichit Jul 10, 2018

Contributor

Done, Updated the code to use native python assert. Also, bought the code duplication down a bit by separating the checks in a separate function.
@nilgoyette and @skoudoro: Thanks.

with open(pjoin(temp_out_dir, qual_fname), 'r') as f:
for line in f:
pass
temp_val = line

This comment has been minimized.

@nilgoyette

nilgoyette Jul 5, 2018

Contributor

So, you're trying to read the last line of the file? If the file is not huge, I think readlines()[-1] would make your intent clearer.

This comment has been minimized.

@parichit

parichit Jul 5, 2018

Contributor

@nilgoyette: Thanks for this one, you are absolutely right, I could have done that. Will update the code.

This comment has been minimized.

@parichit

parichit Jul 6, 2018

Contributor

Done, It looks better now. I did not know that the structure returned by readlines() can be directly index like readlines()[-1], thanks for this one.

test_translation()
test_rigid()
test_affine()
test_err()

This comment has been minimized.

@nilgoyette

nilgoyette Jul 5, 2018

Contributor

We usually use

if __name__ == '__main__':
    run_module_suite()

This comment has been minimized.

@parichit

parichit Jul 5, 2018

Contributor

@nilgoyette: Thank again for this suggestion. Here I am a bit confused, please correct me if I am wrong. From what I understand is that during Travis checks, it automatically looks for all the files starting from 'test_' and executes them. I had that syntax (that you have suggested) earlier but then I removed it because I came to know that I don't need it for the test to execute.

However, I will definitely look into it if changing this one will help in both running the test independently and also in the travis checks.

Thanks again.

This comment has been minimized.

@nilgoyette

nilgoyette Jul 6, 2018

Contributor

You're right, Travis and other tools don't need the run_module_suite(). However, it's practical for us when we run this file in command line or with the play button in our favorite IDE.

This comment has been minimized.

@parichit

parichit Jul 6, 2018

Contributor

Using run_module_suite() now. Thanks.

This comment has been minimized.

@nilgoyette

nilgoyette Jul 10, 2018

Contributor

Good thing you're using run_module_suite() but why are you still calling test_com(), test_translation(), etc.?

This comment has been minimized.

@skoudoro

skoudoro Jul 10, 2018

Member

I was confused too @nilgoyette but when you look at the indentation, these calls are inside test_image_registration() function and call local functions.

out_quality='trans_q.txt')

dist = read_distance('trans_q.txt')
npt.assert_equal('%.2f' % dist, '%.2f' % -0.3953547764454917)

This comment has been minimized.

@nilgoyette

nilgoyette Jul 5, 2018

Contributor

Isn't this a "almost_equal"? It kind of look like one but it's complex and hard to read.

This comment has been minimized.

@parichit

parichit Jul 6, 2018

Contributor

Thanks for this one, yes you are correct, here almost_equal() improves the code readability.


""" Function for the center of mass based image
registration.

This comment has been minimized.

@nilgoyette

nilgoyette Jul 5, 2018

Contributor

I think it fits on 79 characters line.

This comment has been minimized.

@parichit

parichit Jul 6, 2018

Contributor

Yup, fits within 70 characters. Done. Thanks for pointing this out.

return transformed, img_registration.affine

def translate(self, static, static_grid2world, moving,

This comment has been minimized.

@nilgoyette

nilgoyette Jul 5, 2018

Contributor

There's clearly a pattern here. translate, rigid and affine all have the ~same block of code. Only the transform type changes. Maybe you could create a common function for these functions?

This comment has been minimized.

@parichit

parichit Jul 6, 2018

Contributor

@nilgoyette: You are correct, there is clearly a pattern here but the thing is that this pattern can get a bit more complicated depending on how the user wants to run the analysis. So, I simply separated each type of registration in its own function. These functions, in turn, are calling other dependent functions to generate some intermediate results (depending on the type of analysis the user wants to perform.) So, translate can call center of mass or rigid can call translate and center of mass or don't call them at all.

With a single function to abstract the low-level details, it will become a bit more redundant code because it will also call different functions (like it's being done now).

But, I am thinking about this one more, if there is a way to further abstract away the code then I surely will do that. It will structure the code even more nicely.

Thanks.

This comment has been minimized.

@parichit

parichit Jul 9, 2018

Contributor

I have tried to reduce the code duplication by moving the common code from (translate, rigid and affine) functions in a separate function. Thanks for this review.

self.check_dimensions(static, moving)

if transform.lower() == 'com':

This comment has been minimized.

@nilgoyette

nilgoyette Jul 5, 2018

Contributor

Instead of writing transform.lower() 4 times, you could put it in a variable.

This comment has been minimized.

@parichit

parichit Jul 5, 2018

Contributor

@nilgoyette: Yes, you are right. Thanks for this one. Much appreciate it. Will update the code.

This comment has been minimized.

@parichit

parichit Jul 6, 2018

Contributor

Done.

"""
Load the data from the input files and store into objects.
"""

This comment has been minimized.

@nilgoyette

nilgoyette Jul 5, 2018

Contributor

It's the first time I see random comments in """ I thought it was illegal because it's supposed to be reserved for documentation. I was wrong. IMO, I think we should stick to normal # comments.

This comment has been minimized.

@parichit

parichit Jul 5, 2018

Contributor

@nilgoyette: I think I just overlooked this one. It makes sense to follow the '#' based commenting for this one. Really appreciate this. Thanks.

@@ -20,12 +31,127 @@ def test_reslice():
out_path = reslice_flow.last_generated_outputs['out_resliced']
out_img = nib.load(out_path)
resliced = out_img.get_data()


npt.assert_equal(resliced.shape[0] > volume.shape[0], True)
npt.assert_equal(resliced.shape[1] > volume.shape[1], True)
npt.assert_equal(resliced.shape[2] > volume.shape[2], True)
npt.assert_equal(resliced.shape[-1], volume.shape[-1])

This comment has been minimized.

@nilgoyette

nilgoyette Jul 10, 2018

Contributor

@skoudoro and others, do you have an opinion on the use of assert? I don't like statements much, but I like assert_equal(wathever, True) even less.

test_translation()
test_rigid()
test_affine()
test_err()

This comment has been minimized.

@nilgoyette

nilgoyette Jul 10, 2018

Contributor

Good thing you're using run_module_suite() but why are you still calling test_com(), test_translation(), etc.?

from dipy.align.transforms import (TranslationTransform3D, RigidTransform3D,
AffineTransform3D)
from dipy.io.image import save_nifti, save_affine_matrix, \
save_quality_assur_metric

This comment has been minimized.

@nilgoyette

nilgoyette Jul 10, 2018

Contributor

I don't know if DiPy has a standard or not, but since it's your own code, can you stick to one way? Either import () or import \, but not both.

This comment has been minimized.

@parichit

parichit Jul 10, 2018

Contributor

Done, Thanks. Actually, part of this file was contributed was someone else earlier and so I did not fiddle with the parts that were already there but you have a valid point, I also support code consistency and standard. It's better to just stick to a single standard rather than using different ones. Good catch, I did not even notice it until you pointed this out.

"""
moved, affine = self.center_of_mass(static, static_grid2world,
moving, moving_grid2world)

This comment has been minimized.

@nilgoyette

nilgoyette Jul 10, 2018

Contributor

Looks like you're not using moved here and below. Simply use _, affine = ...

This comment has been minimized.

@parichit

parichit Jul 10, 2018

Contributor

Done. I forgot to take advantage of this feature of Python :) Thanks for reminding me.

"""
if progressive:
moved, affine, xopt, fopt = self.rigid(static, static_grid2world,

This comment has been minimized.

@nilgoyette

nilgoyette Jul 10, 2018

Contributor

Idem. When not using an answer, use _

This comment has been minimized.

@parichit

parichit Jul 10, 2018

Contributor

Done.

affreg, params0, transform, starting_affine):

""" Function for translation based registration.

This comment has been minimized.

@nilgoyette

nilgoyette Jul 10, 2018

Contributor

Copy pasting is often a trap in programming.

This comment has been minimized.

@parichit

parichit Jul 10, 2018

Contributor

Yeah, totally agree. Sorry for this. Updated it. :)

static_grid2world,
moving_grid2world,
starting_affine=starting_affine,

This comment has been minimized.

@nilgoyette

nilgoyette Jul 10, 2018

Contributor

I'm not too sure where optimize is defined so I'm sure, but check if you actually need to specify the name. starting_affine=starting_affine, is kind of useless to read.

"""

def perform_transformation(self, static, static_grid2world, moving,

This comment has been minimized.

@nilgoyette

nilgoyette Jul 10, 2018

Contributor

I understand now why you didn't create this function on the first time. Yes, there was some code copy, but there's so much parameters and comments that you're probably not saving much space. If you prefer your old version, you can revert your changes. As you wish, I won't complain on this matter anymore :)

This comment has been minimized.

@parichit

parichit Jul 10, 2018

Contributor

Yeah, I tried and thought about somehow reducing the duplication but figured that it's just not possible given the current specification of parameters and different analysis that can be done with this workflow. If I remove the code from one part then it has to come up in some other part (and it's the same code), Phewww. But, I haven't given up, still thinking about how to make it less redundant. :)

@@ -954,7 +954,7 @@ def _init_optimizer(self, static, moving, transform, params0,

def optimize(self, static, moving, transform, params0,
static_grid2world=None, moving_grid2world=None,
starting_affine=None):
starting_affine=None, ret_metric=False):

This comment has been minimized.

@parichit

parichit Jul 10, 2018

Contributor

@nilgoyette: Thanks for pointing out the thing about 'starting_affine'. Here is the optimize function definition, So the 'starting_affine' is there as an optional parameter but I think it is position independent so passing a value to that argument won't work (please correct me if I am wrong). So, that's why I stick to the name 'starting_affine'. But I have changed it to 'starting_affine=affine' from what it was earlier (aesthetically it looks better now). A very good point though, I really appreciate all the useful comments that you have made, it really helped me to improve the quality of the code. Thanks.

@Garyfallidis

This comment has been minimized.

Copy link
Member

Garyfallidis commented Jul 30, 2018

This PR needs rebasing on top of master.


"""
Importing the flow runner method and the ImageRegistrationFlow
method from the workflows.

This comment has been minimized.

@Garyfallidis

Garyfallidis Jul 30, 2018

Member

comment redundant

This comment has been minimized.

@parichit

parichit Jul 30, 2018

Contributor

Thanks for pointing this out. Made the changes.

"""
This is the method that will wrap everything that is needed to make a flow
command line ready then run it.
"""

This comment has been minimized.

@Garyfallidis

Garyfallidis Jul 30, 2018

Member

comment redundant

This comment has been minimized.

@parichit

parichit Jul 30, 2018

Contributor

Removed the comment. Thank you.

The distance between the registered images.
"""
np.savetxt(fname, xopt, header="Optimal Parameter metric")
with open(fname, 'a') as f:

This comment has been minimized.

@Garyfallidis

Garyfallidis Jul 30, 2018

Member

Is 'a' the correct parameter or should it be 'w'?

This comment has been minimized.

@Garyfallidis

Garyfallidis Jul 30, 2018

Member

No, that is correct. Ignore.

@parichit

This comment has been minimized.

Copy link
Contributor

parichit commented Jul 30, 2018

@Garyfallidis: Kindly take a brief look at the parameter description in the run method, I have tried to write the description as concise as possible but I think that it can be a bit verbose, i just don't the correct terminology to use.

Thanks.

L1 = (-3 + np.sqrt(1 + 8 * n_coeffs)) / 2
L2 = (-3 - np.sqrt(1 + 8 * n_coeffs)) / 2
return np.int(max([L1, L2]))
# L2 is negative for all positive values of n_coeffs, so we don't

This comment has been minimized.

@Garyfallidis

Garyfallidis Jul 30, 2018

Member

Not sure why this is here. Seems irrelevant to registration. I suppose we need to rebase?

This comment has been minimized.

@parichit

parichit Jul 30, 2018

Contributor

You were right, I checked again and the earlier rebase was not complete, there were conflicts remaining. I did a rebase again (after pulling the latest changes from the remote origin) and merged the conflicts, manually. This branch should be okay now :)

@skoudoro

This comment has been minimized.

Copy link
Member

skoudoro commented Aug 21, 2018

closing in favor of #1604

@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