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

BUG: plot_2d_diffeomorphic_map #541

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@grlee77
Contributor

grlee77 commented Jan 5, 2015

This pull request fixes two minor bugs in plot_2d_diffeomorphic_map that I came across when playing around with the new align functionality (which is great by the way!):

  • If fname is set, but show_figure is false, the call to plt.savefig() should not be performed
  • The docstring states that direct_grid_shape and inverse_grid_shape should be a tuple, but if a tuple is used, the call to mapping.transform() raises an exception because the warp functions expect a numpy.int32 array.

To fix this, I put a conversion to numpy.int32 within the transform method itself, because it is also an issue there if the provided sampling_shape array is not of type numpy.int32.

I also removed a couple of unused mgrid variables from plot_2d_diffeomorphic_map and did some PEP8 formatting of regtools.py.

Finally, if interested, I can submit a separate pull request with an enhancement where draw_lattice_2d is generalized to draw_lattice_nd and there is a plot_diffeomorphic_map() function that works for either 2d or 3d cases.

@omarocegueda

This comment has been minimized.

Contributor

omarocegueda commented Jan 5, 2015

Thank you very much for doing this @grlee77!, it looks all good to me, just waiting until travis finishes its tests before merging. I would really like to see your enhancement for plot_diffeomorphic_map!, how do you plot the deformed lattice? do you plot individual slices of the deformed 3D grid?

@omarocegueda

This comment has been minimized.

Contributor

omarocegueda commented Jan 5, 2015

It looks all good to me and Travis didn't complain. Guys, may I go ahead and merge this, or wait until after the release?
Thanks!

@grlee77

This comment has been minimized.

Contributor

grlee77 commented Jan 5, 2015

For the 3D case, to keep things simple I only implemented computation of the warped grids, and raise an error if show_figure is True.

As far as visualizing the 3D warp fields, there were two ways I tried:

1.) to look at it interactively, OrthoSlicer3D from this nipype pull request works well:
nipy/nibabel#251

Although, ideally it would be nicer to only have grid lines in the two dimensions perpendicular to the axis being shown in the viewer, this would require a customized implementation of the class.

It would also be nice to have a subclass of this that takes two volumes and does red/green color overlays as a 3D version of regtools.overlay_slices()

2.) I have some code that can generate 2D mosaics from a 3D volume, but this is not as visually nice to look at.

@grlee77

This comment has been minimized.

Contributor

grlee77 commented Jan 5, 2015

I will wait until this is merged before putting the pull request for the nd case. Here is what the lattice code looks like, though:

here the slice_array array is used to enable arbitrary lattice dimensions analagous to how the same sort of things is done in functions such as numpy.gradient which support nd inputs.

def draw_lattice_nd(shape, gridspacings):
    r"""Create a regular lattice of black lines on a white background

    Parameters
    ----------
    shape : tuple or list
        size of the lattice along each dimension
    gridspacings : tuple or list
        grid spacing to use along each dimension. Along dimension i, black
        lines occur at np.arange(gridspacings[i], shape[i], gridspacings[i])

    Returns
    -------
    lattice : array
        A white volume (nD array) of the specified `shape`.  Black grid lines
        are spaced at the specified gridspacing along each dimension

    """
    ndim = len(shape)
    if len(gridspacings) != ndim:
        raise ValueError("shape and gridspacing lists must have the same "
                         "length")

    # white background
    lattice = 127 * np.ones(shape, dtype=np.float64)

    # loop over axes, adding black lines at the requested grid spacings
    slice_array = [slice(None)] * ndim
    for sl in range(ndim):
        slice_array[sl] = slice(gridspacings[sl], shape[sl], gridspacings[sl])
        lattice[slice_array] = 0
        slice_array[sl] = slice(None)
    return lattice

Then in the plot_diffeomorphic_map, the previous implementation for 2D:

    # Draw the squares on the output grid
    lattice_out = draw_lattice_2d(
        (inverse_grid_shape[0] + delta) / (delta + 1),
        (inverse_grid_shape[1] + delta) / (delta + 1),
        delta)
    lattice_out = lattice_out[0:inverse_grid_shape[0], 0:inverse_grid_shape[1]]

is replaced by this nd case:

    #Draw the squares on the output grid
    lattice_out = draw_lattice_nd(inverse_grid_shape, [delta, ]*mapping.dim)
@arokem

This comment has been minimized.

Member

arokem commented Jan 5, 2015

I propose to wait with merging this for until after the current release.
The general plan is to release as soon as we can, without further review of
currently pending PRs, except those pertaining to stuff that has already
made it into the current release. We also plan to have another release at
the end of February, so this will follow the current release quickly,
dealing with odds and ends like this. Thanks for your patience with us.

On Mon, Jan 5, 2015 at 3:07 PM, Gregory R. Lee notifications@github.com
wrote:

I will wait until this is merged before putting the pull request for the
nd case. Here is what the lattice code looks like, though:

here the slice_array array is used to enable arbitrary lattice dimensions
analagous to how the same sort of things is done in functions such as
numpy.gradient which support nd inputs.

def draw_lattice_nd(shape, gridspacings):
r"""Create a regular lattice of black lines on a white background Parameters ---------- shape : tuple or list size of the lattice along each dimension gridspacings : tuple or list grid spacing to use along each dimension. Along dimension i, black lines occur at np.arange(gridspacings[i], shape[i], gridspacings[i]) Returns ------- lattice : array A white volume (nD array) of the specified shape. Black grid lines are spaced at the specified gridspacing along each dimension """
ndim = len(shape)
if len(gridspacings) != ndim:
raise ValueError("shape and gridspacing lists must have the same "
"length")

# white background
lattice = 127 * np.ones(shape, dtype=np.float64)

# loop over axes, adding black lines at the requested grid spacings
slice_array = [slice(None)] * ndim
for sl in range(ndim):
    slice_array[sl] = slice(gridspacings[sl], shape[sl], gridspacings[sl])
    lattice[slice_array] = 0
    slice_array[sl] = slice(None)
return lattice

Then in the plot_diffeomorphic_map, the previous implementation for 2D:

# Draw the squares on the output grid
lattice_out = draw_lattice_2d(
    (inverse_grid_shape[0] + delta) / (delta + 1),
    (inverse_grid_shape[1] + delta) / (delta + 1),
    delta)
lattice_out = lattice_out[0:inverse_grid_shape[0], 0:inverse_grid_shape[1]]

is replaced by this nd case:

#Draw the squares on the output grid
lattice_out=draw_lattice_nd(inverse_grid_shape, [delta,]*mapping.dim)


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

@grlee77

This comment has been minimized.

Contributor

grlee77 commented Jan 5, 2015

Hi @arokem

That is fine by me, although I would consider merging this present bug fix pull request and just wait on the nd implementation until the next release. There is no new functinality implemented in the present pull request.

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Jan 5, 2015

Hi @grlee77 thank you for your PR. This is indeed very useful. But we have been delaying the release again and again and again. Now having more than 100 PRs in one release. It's about time to say no to including more and just release what we already have. We want also to change our release circle so we release more often. So, this shouldn't take long to be released. I hope you can understand this. Thank you again for your PR.

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Jan 5, 2015

I would argue that bug-fixes should go in. If they cause pain we can always release off the previous commit.

I'm happy to take charge of the release.

@arokem

This comment has been minimized.

Member

arokem commented Jan 6, 2015

After some more thought, I have changed my mind here. I think that we
should merge this one before making the release, since it's simple and
fixes an obvious bug. That said, I think that we should make the release
tomorrow after merging this, without any further merges. In particular,
unless it fixes that buildbot bug, I think that #540 should wait. How does
that sound?

On Monday, January 5, 2015, Matthew Brett <notifications@github.com
javascript:_e(%7B%7D,'cvml','notifications@github.com');> wrote:

I would argue that bug-fixes should go in. If they cause pain we can
always release off the previous commit.

I'm happy to take charge of the release.


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

@samuelstjean

This comment has been minimized.

Contributor

samuelstjean commented Jan 6, 2015

No #539 fix for the deconv errors?

@arokem

This comment has been minimized.

Member

arokem commented Jan 6, 2015

I vote no. There are many ways in which dipy is not perfect. Only some of
them are addressed in this release. That's why we keep working on this. A
year between releases is way too long, and if we aim to make this perfect
before we ever release, we will never make a release. We should deal with
some of the flaws right now, and keep making it better, starting tomorrow

On Monday, January 5, 2015, Samuel St-Jean notifications@github.com wrote:

No #539 #539 fix for the deconv errors?


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

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Jan 6, 2015

It would be nice to have a test of the sampling_shape fix.

@arokem arokem referenced this pull request Jan 6, 2015

Closed

TST: Testing regtools #542

@arokem

This comment has been minimized.

Member

arokem commented Jan 6, 2015

You mean something like this?

#542

On Mon, Jan 5, 2015 at 7:25 PM, Matthew Brett notifications@github.com
wrote:

It would be nice to have a test of the sampling_shape fix.


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

@arokem

This comment has been minimized.

Member

arokem commented Jan 6, 2015

Hey @matthew-brett - if #542 properly addresses your issue, please merge this one here, and that other one as well.

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Jan 6, 2015

Guys - I combined this with #542 and added another fix, did some more work - see #543

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Jan 6, 2015

I am closing this PR as this PR has been included into #543.

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