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

WIP: Workflow for syn registration #673

Closed
wants to merge 5 commits into from

Conversation

arokem
Copy link
Contributor

@arokem arokem commented Jul 2, 2015

Heads up - I am trying to write a little script that simplifies the SyN registration into just one line of code:

from dipy.workflows.align import syn_registration
warped = syn_registration(moving, static)

The idea is to allow full control of things through kwargs, but provide some reasonable defaults as a starting point.

I am making a PR from this to solicit feedback and discuss the overall approach. Is this a good way to construct an easier-to-use API for functionality that otherwise requires several lines of code?

@omarocegueda
Copy link
Contributor

Hi @arokem,
It's great that you brought this point to discussion!, I think this is a good script for a basic registration (I guess most people will use the tools this way). People may also be interested in mapping a set of annotations made on top of the moving image instead of only warping the moving image itself (using nearest neighbor interpolation), or map a different image that is already co-registered with the moving image (using tri-linear interpolation).

Maybe it would be good to specify (in addition to the input images) a list of images to be warped and the interpolation required for each one of them. The default behavior would be precisely what you do in your script.

Another question that I wanted to discuss with you guys is how to efficiently save the mapping object to disk? Have you ever tried to save and load as a python object? it's very slow because it maybe hundreds of MB! (but it can be efficiently done by directly writing the buffers to disk).

@rfdougherty
Copy link

Hi @omarocegueda,
It seems that the mapping object is primarily gridded data (the forward and backward deformation fields) and associated affines. Maybe you can put these into one or two nifti files for optimal portability?

@omarocegueda
Copy link
Contributor

Thanks @rfdougherty!, yes, the deformation fields are just two 4D arrays (a 3D vector for each voxel). Sorry I don't know much about the nifti format, is it possible to save several arrays (with different shapes) in one single nifti file (so we can save both the deformation fields and the matrices in one single file)?

@arokem
Copy link
Contributor Author

arokem commented Jul 2, 2015

I don't think that you can save more than one block of data in a single nifti file, and I am not sure that would even make sense. Just to see that I understand what is meant: the deformation fields you mentioned are the 4D arrays. What do you mean by 'matrices' in your previous comment? If you mean the affine matrices, mapping the voxel coordinates to the scanner space (or some-such) then you can definitely do that, but not the two differently-dimensioned 4D deformation fields in the same file.

I think that you should be able to save the two 4D arrays in separate nifti files, each with its own associated affine matrix. We could even add it to this code, to return the nifti objects, instead of the array:

    return (new_image, nib.Nifti1Image(np.array(mapping.forward), moving_affine), 
        nib.Nifti1Image(np.array(mapping.backward), static_affine))

Would that make more sense?

@omarocegueda
Copy link
Contributor

What I would like to do is to save the DiffeomorphicMap object to disk in case I would like to use it later without the need to re-run the registration, and I would like to save one single file to avoid making a mess with several separate files for each DiffeomorphicMap I have.

The DiffeomorphicMap represents a function that maps points from its "domain" to its "co-domain". The domain is the set of points in the static image and the co-domain is the set of points in the moving image. Since the map operates in physical space, we need to remember the grid2world matrix from both domain and co-domain grids (and we also need to remember what the shapes of those grids are). Also, since the deformation fields are given in a grid too, we also need to know the grid2world matrix of that grid. Then we need to store those matrices, grid shapes, and the deformation fields, and it would be desirable to save everything in one single file. We could save both deformation fields in the nifti file with a 5D volume with shape (nx,ny,nz,3,2), but how can we save the rest of the information?

@arokem arokem changed the title WIP: Workflow for syn registration. Smoke-tested only for now. WIP: Workflow for syn registration Jul 4, 2015
@Garyfallidis
Copy link
Contributor

Hi @arokem and all. There are two BIG problems with this PR.

First, there is a misunderstanding of what gets into workflows. In workflows we have functions that get as input filepaths from the disk not objects or numpy arrays. These should go to the original module sites.

Second there is a misunderstanding of what type of testing we should have in workflows. In worflows a numpy testing like system is irrelevant. This is where we need an offline testing system doing end to end quality assurance.

So here is what I think we should do as we discussed in OHBM. Sorry if I was not clear then. In workflows you will have functions getting input files from the disk and parameters. And the inputs will be able to give you multiple files. See a WIP example here

https://github.com/matthieudumont/dipy/blob/introducing_workflows/dipy/workflow/segment.py

Here is an example of a correct workflow for this problem
syn_registration(input_moving_files, input_fixed_files, output_dir, param1, param2, param3, ...)

Inside this function you will be able to process and register many files.
This can then be called by a short python file in dipy/bin

I would suggest you to wait for the first PR about the workflows from @matthieudumont which will give you a better idea of what we are proposing and the fvtk_2.0 PR which will help with the end-to-end quality assurance testing.

Is that reasonable?

@arokem
Copy link
Contributor Author

arokem commented Jul 4, 2015

As you mentioned, we did have a brief conversation during HBM, but we
obviously took different things from that conversation. I was under the
impression that the workflows module would be a good place to put
simplified scripting API for this kind of thing, and I seem to remember you
agreeing with me about that, which is why I put this code here in the first
place. I put in this PR now, because I found myself writing a bunch of
boilerplate to do things that really should be one-liners, so I went ahead
with this. I don't mind putting it in another module if we all agree that's
better. For now, I started an align.api submodule (see recent commit).
How's that for a solution?

I also generally prefer to have discussions (like this one) on very
early stages of a PR, before showing up with many thousands of lines of
code. Maybe @matthieudumont could put up a PR from that branch you pointed
out, so that you can both have a chance to say a bit more about what you
envision for workflows? I understand that you want to only have strings
of file-system locations as inputs to these functions, but I think that the
could be written to allow lists of strings, lists of file objects, lists of
nifti objects, or even lists of arrays without too much modification to the
API you are writing. Of course only one of these would make sense in a CLI,
but others might make more sense in a scripting API. There might be
something to gain from a workflows module that can serve both as a
mediator for CLI, and for simplified scripting?

On Sat, Jul 4, 2015 at 10:41 AM, Eleftherios Garyfallidis <
notifications@github.com> wrote:

Hi @arokem https://github.com/arokem and all. There are two BIG
problems with this PR.

First, there is a misunderstanding of what gets into workflows. In
workflows we have functions that get as input filepaths from the disk not
objects or numpy arrays. These should go to the original module sites.

Second there is a misunderstanding of what type of testing we should have
in workflows. In worflows a numpy testing like system is irrelevant. This
is where we need an offline testing system doing end to end quality
assurance.

So here is what I think we should do as we discussed in OHBM. Sorry if I
was not clear then. In workflows you will have functions getting input
files from the disk and parameters. And the inputs will be able to give you
multiple files. See a WIP example here

https://github.com/matthieudumont/dipy/blob/introducing_workflows/dipy/workflow/segment.py

Here is an example of a correct workflow for this problem
syn_registration(input_moving_files, input_fixed_files, output_dir,
param1, param2, param3, ...)

Inside this function you will be able to process and register many files.
This can then be called by a short python file in dipy/bin

I would suggest you to wait for the first PR about the workflows from
@matthieudumont https://github.com/matthieudumont which will give you a
better idea of what we are proposing and the fvtk_2.0 PR which will help
with the end-to-end quality assurance testing.

Is that reasonable?


Reply to this email directly or view it on GitHub
#673 (comment).

@Garyfallidis
Copy link
Contributor

Hi @arokem, all interesting ideas. I think we don't need an api submodule. But let me comment directly in your PR. I think it will be easier.

import vector_fields as vfu


def syn_registration(moving, static, metric=CCMetric(3),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function can go directly in dipy.align.imwarp there is no reason to create a new api.py submodule. Why because then we will always have an ambiguous question to answer. Which is what goes to api.py and what doesn't. This also creates a problem with the standard way of writing scripts which is what we were using so far.

I would write it in the following way:
This function that you have here would go to dipy.align.imwarp in a bit different form:

def syn_registration(static, static_affine, moving, moving_affine, metric='CC', dim=3, level_iters, other_params ...):

Parameters
----------------
static : ndarray

Notice here that I am bringing back the affines in the parameters because that is giving us higher flexibility to play with different coordinate transforms. Also, notice that metric is now just a string and the actual object will be created inside the syn_registration object. There is no need to create this object outside this function. Also, notice that dim is now a new parameter. This is because in most cases we will use this in 3D images so this parameter can be easily set to default of 3 and then just change the metric name.

Okay, so now that we have a very simple function which would run with one static and one moving image and would work smoothly from our standard scripts we can go and see how the workflow interface would be.

So, in dipy.workflows.align we can have now a function called for example

def syn_registration_interface(inputs_static, inputs_moving, outputs_moved, other_params ...):
"""
Parameters
----------------
inputs_static : str (filepath) of File or list of str or list of File
...
"""
# binds all inputs together and creates the output files

This function now has the capability to run the syn_registration function which we created before with multiple input files and outputs. So, it connects one or more collections of inputs to one or more outputs. Now, that we have the interface function that can run with multiple files, let's go and create the command line interface in dipy/bin/dipy_syn

from dipy.workflow.segment import syn_registration_interface
from dipy.fixes import argparse as arg
parser = arg.ArgumentParser()
parser.add_argument('input_static', action='store')
parser.add_argument('input_moving', action='store')
parser.add_argument('output_directory', action='store')

if __name__ == "__main__":
    params = parser.parse_args()
    syn_registration_interface(params.input_static, params.input_moving, params.output_directory, params.other ...)

Now that we have this very thin cmd interface we can go in a terminal and run for example

>>> dipy_syn -i Data/exp10/atlas/static.nii.gz, Data/exp10/subj*/moving.nii.gz -o Data/exp10/subj*/moved.nii.gz --metric "CC" -iters ...

This is now beautiful because we have a very thin CLI which also is using the power of bash which allows you to have * and ? etc and process multiple subjects at the same time.

Also, we can use the same interface from a website or an ipython notebook too. The CLI is only one option.

How does that sound?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have to run for a bit but in a couple of hours will also give you another more clarifying example. See you soon! :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here you have it.
Here is a dummy example. I am assuming that someone needs to do a very simple processing. For example combine fa and t1 from the same subjects, do some processing with them and then create some output in an output_dir.
Here is the very thin call from the CLI
https://github.com/Garyfallidis/dipy/blob/interfaces/bin/dipy_dummy
Here is the actual interface with some example usage in the main from other scripts, web interfaces etc.
https://github.com/Garyfallidis/dipy/blob/interfaces/dipy/workflows/dummy.py

Let me know what you think.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow - thanks for taking the time with that!

I am still chewing on this for a bit more, and I am traveling to Scipy
tomorrow, so I might be delayed in working on this more, but this gives a
solid basis for discussion!

On Sun, Jul 5, 2015 at 2:57 PM, Eleftherios Garyfallidis <
notifications@github.com> wrote:

In dipy/align/api.py
#673 (comment):

@@ -0,0 +1,51 @@
+import numpy as np
+import nibabel as nib
+
+from imwarp import SymmetricDiffeomorphicRegistration
+from imwarp import DiffeomorphicMap
+from metrics import CCMetric
+import vector_fields as vfu
+
+
+def syn_registration(moving, static, metric=CCMetric(3),

Here you have it.
Here is a dummy example. I am assuming that someone needs to do a very
simple processing. For example combine fa and t1 from the same subjects, do
some processing with them and then create some output in an output_dir.
Here is the very thin call from the CLI
https://github.com/Garyfallidis/dipy/blob/interfaces/bin/dipy_dummy
Here is the actual interface with some example usage in the main from
other scripts, web interfaces etc.

https://github.com/Garyfallidis/dipy/blob/interfaces/dipy/workflows/dummy.py

Let me know what you think.


Reply to this email directly or view it on GitHub
https://github.com/nipy/dipy/pull/673/files#r33899405.

@arokem
Copy link
Contributor Author

arokem commented Jul 15, 2015

Just a short note here to say that I will wait for #654 to land before continuing work on this. Upon some further thinking, I believe that this scripting API and pipeline should at least include the option to perform a pre-alignment, based on an affine registration, followed by the SyN registration.

@arokem
Copy link
Contributor Author

arokem commented Oct 21, 2016

I think this one is superseded by the work that @matthieudumont is doing on workflows, so I am going to close this.

@arokem arokem closed this Oct 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants