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

Fix utils docs and affine #352

Merged
merged 5 commits into from Jun 25, 2014

Conversation

Projects
None yet
4 participants
@MrBago
Contributor

MrBago commented Apr 23, 2014

I think this should address #322 and #351.

Because I had to move most if _utils back into utils, sorry I'm the one who broke this in the first place, this PR might seem like a lot, but it should be easier to review by looking at the individual commits. The first two commit are pretty strait forward and the third commit is pretty much just a copy and past.

@jchoude could you take a look at the functions that use the affine argument and see if we could make it any more clear in the docs that this argument is required.

@@ -9,40 +9,641 @@
Some functions in this module use an affine matrix to represent the coordinate
system associated with the points of a streamline. Dipy uses a similar

This comment has been minimized.

@jchoude

jchoude May 27, 2014

Contributor

Small comment, I know this was already like this, but it would read better to say "Dipy uses a convention similar to nifti files..."

The shape of the volume to be returned containing the streamlines
counts
voxel_size :
This argument is deprecated.

This comment has been minimized.

@jchoude

jchoude May 27, 2014

Contributor

If this argument is deprecated, we should make affine mandatory (by removing the default value).

This comment has been minimized.

@MrBago

MrBago Jun 5, 2014

Contributor

To maintain backwards compatibility we need density_map(sl, voxdim, vs) to mean density_map(sl, voxdim, voxel_size=vs). I think nipype uses density map somewhere and users (like me) have scripts that haven't yet been updated. I'd like to have 1 release with backwards compatibility then we can remove voxel_size all together.

An image volume with an integer data type, where the intensities in the
volume map to anatomical structures.
voxel_size :
This argument is deprecated.

This comment has been minimized.

@jchoude

jchoude May 27, 2014

Contributor

Same thing about deprecation versus making affine mandatory.

"""
# Error checking on label_volume
kind = label_volume.dtype.kind
labels_possitive = ((kind == 'u') or

This comment has been minimized.

@jchoude

jchoude May 27, 2014

Contributor

Possible small type: possitive should have only an "s".

total of 8 seeds per voxel.
voxel_size :
This argument is deprecated.
affine : array, (4, 4)

This comment has been minimized.

@jchoude

jchoude May 27, 2014

Contributor

Same comment regarding the deprecation versus default param.

@jchoude

This comment has been minimized.

Contributor

jchoude commented May 27, 2014

Overall, all seems good. A few small points:

  • I think we should write somewhere (where, I don't know) that, if streamlines are already loaded in "real-world coordinates", the affine can (and should be set) to np.eye(4). It's kind of said in your explanation of the affine, but making this part explicit would probably help some users.
  • Should we provide a function to create an affine from a list of voxel sizes? It could maybe be useful for someone loading their streamlines without having a reference anatomy to get the affine, but that knows that the streamlines were generated in a (e.g.) 2x2x2 volume. That way, the could be able to pass an affine to the functions that need one, without being lost.
@MrBago

This comment has been minimized.

Contributor

MrBago commented Jun 5, 2014

An affine of eye(4) means the streamlines are in voxel coordinates, not real world coordinates. I've done my best to describe this at the beginning of the module, but I'm always a happy to get help writing documentation :).

It's in general hard to create an affine from limited information. The affine is not arbitrary and using the wrong affine will result in streamlines being outside the image for example. If the streamlines are loaded from a known file type with good header information, we can build the affine from that. For example affine_for_trackvis build the affine for streamlines read from a trackvis file. Within dipy methods that create streamlines produce the affine as well so there should be no issue there.
The affine_for_trackvis function could use a little work, but we have a basic version already.

@MrBago

This comment has been minimized.

Contributor

MrBago commented Jun 11, 2014

Any objections to merging this? Is anyone still planning on looking over it?

@jchoude

This comment has been minimized.

Contributor

jchoude commented Jun 11, 2014

Yes sorry for the delay.

Regarding your comment about the affine of eye(4) meaning the streamlines are in voxel coordinates, this is true. However, let's say my original data was in a 1x1x1 resolution, and some preprocessing put everything right up to the origin... In that case, an affine of eye(4) would also fit with the world coordinates, except if I really misunderstood something, which can happen :)

My case was that it could be written in the doc that, if streamlines correspond to the scenario I just described, an affine of eye(4) could be used. Of course, that's just one use-case.

As for the helper function create an affine only from voxel sizes, I agree that's it maybe too much, and could cause havoc for users not knowing what they are doing.

I also agree with your point about the deprecation cycle. Should this be written in a doc somewhere, saying what will be deprecated and when? Just so we remember...

@MrBago

This comment has been minimized.

Contributor

MrBago commented Jun 11, 2014

On Wed, Jun 11, 2014 at 6:09 AM, Jean-Christophe Houde <
notifications@github.com> wrote:

Regarding your comment about the affine of eye(4) meaning the streamlines
are in voxel coordinates, this is true. However, let's say my original data
was in a 1x1x1 resolution, and some preprocessing put everything right up
to the origin... In that case, an affine of eye(4) would also fit with the
world coordinates, except if I really misunderstood something, which can
happen :)

The docs say that "voxel coordinates" means eye(4), eye(4) can mean voxel
coordinates, or the situation you describe above. I don't think there is
special need to discuss though because in this special case where the data
is re-sampled onto a 1mm grid and the origin has been shifted, the user is
likely to be reading the affine from a file header anyways so will not
likely be manipulating the affine manually. The only reason I made a
special note about "voxel coordinates" is that we had some older code in
dipy which returned streamlines in "voxel coordinates" without an affine so
I wanted to make sure that users knew what to do in those situations. Most
of that code is/has been phased out so I'm happy to drop the note from the
docs all together. Alternatively I'm happy to merge any suggestions that
make the docs more clear.

I also agree with your point about the deprecation cycle. Should this be

written in a doc somewhere, saying what will be deprecated and when? Just
so we remember...

We could consider this the doc :). There is a deprecation warning raised
when voxel_size is used, a note in the private function which raises that
warning might be a good place to keep track of our plan for phasing out
this argument. We'll likely have more deprecation discussion when we get
close to a release.


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

@jchoude

This comment has been minimized.

Contributor

jchoude commented Jun 12, 2014

All good for me. Can be merged!

@arokem

This comment has been minimized.

Member

arokem commented Jun 12, 2014

Hey @MrBago - if you rebase this, I can go ahead and merge. Ping me when you are set.

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Jun 18, 2014

@MrBago rebase please! GGT!

@arokem

This comment has been minimized.

Member

arokem commented Jun 24, 2014

@MrBago - just another reminder: please rebase this, so that we can complete the merge. I tried to do this myself twice, but I must be missing something, because I keep getting test errors after the rebase. Thanks!

@MrBago

This comment has been minimized.

Contributor

MrBago commented Jun 25, 2014

Yay it's passing now! Sorry for the delay. This should be ready to merge now.

arokem added a commit that referenced this pull request Jun 25, 2014

@arokem arokem merged commit da656ca into nipy:master Jun 25, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment