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

Seeds from mask random #614

Merged
merged 17 commits into from Apr 29, 2015

Conversation

Projects
None yet
4 participants
@ajnirp
Contributor

ajnirp commented Mar 24, 2015

Code + tests for #591

@ajnirp

This comment has been minimized.

Contributor

ajnirp commented Mar 24, 2015

I stripped trailing spaces before committing, so there are some extra changes in there. The relevant changes are confined to seeds_from_mask() and test_seeds_from_mask()

@MrBago

This comment has been minimized.

Contributor

MrBago commented Mar 24, 2015

Thanks @wenderen this looks nice. Can I suggest that we add the feature as a separate function? I don't think we gain much by having it be in seeds_from_mask. For example, I don't know if using len 3 density input makes sense for random seeds, maybe instead of density a random_seeds_from_mask function could have a seeds_per_voxel argument. We could add the new in a few examples and in the see also section of seeds_from_mask to help users find it.

@ajnirp

This comment has been minimized.

Contributor

ajnirp commented Mar 24, 2015

I agree, having a separate function makes sense. I'll make the changes and push again.

@ajnirp

This comment has been minimized.

Contributor

ajnirp commented Mar 24, 2015

The build check for random_seeds_from_mask is failing because the random outputs from the Travis job are different from the example outputs I got - as they should be :)

Should I remove the examples from random_seeds_from_mask or keep them there, but comment them out? Or just let it be?

[ 0.5 , 0.75 , 0.5 ],
[ 0.5 , 0.25 , 0.83333333],
[ 0.5 , 0.75 , 0.83333333]])
>>> random_seeds_from_mask(mask, seeds_per_voxel=1, voxel_size=[1,1,1])

This comment has been minimized.

@MrBago

MrBago Mar 25, 2015

Contributor

Why is the example for seeds_from_mask calling random_seeds_from_mask? Shouldn't this be in the latter function?

This comment has been minimized.

@ajnirp

ajnirp Mar 25, 2015

Contributor

Typo, sorry. Fixed.

# Use affine to move seeds int real world coordinates
seeds = np.dot(seeds, affine[:3, :3].T)
seeds += affine[:3, 3]
elif voxel_size is not None:

This comment has been minimized.

@MrBago

MrBago Mar 25, 2015

Contributor

We don't need to worry about voxel size here, we left it in the other functions for backwards compatibility, but we don't need to add it to new functions.

This comment has been minimized.

@ajnirp

ajnirp Mar 25, 2015

Contributor

Removed

seeds = where + grid - .5
seeds = asarray(seeds)
# Apply the spacial transform

This comment has been minimized.

@arokem

arokem Mar 25, 2015

Member

typo: "spacial" => "spatial"

This comment has been minimized.

@ajnirp

ajnirp Mar 25, 2015

Contributor

Done

# Apply the spacial transform
if affine is not None:
# Use affine to move seeds int real world coordinates

This comment has been minimized.

@arokem

arokem Mar 25, 2015

Member

"int" => "into"

This comment has been minimized.

@ajnirp

ajnirp Mar 25, 2015

Contributor

Done

ajnirp added some commits Mar 25, 2015

@ajnirp

This comment has been minimized.

Contributor

ajnirp commented Mar 29, 2015

(ping)

--------
>>> mask = np.zeros((3,3,3), 'bool')
>>> mask[0,0,0] = 1
>>> random_seeds_from_mask(mask, seeds_per_voxel=1, voxel_size=[1,1,1])

This comment has been minimized.

@arokem

arokem Mar 29, 2015

Member

This doctest is currently failing, because the function does not have a voxel_size kwarg. Is knowing the voxel size important for this function? If it's not, can you remove this kwarg from the doctest?

This comment has been minimized.

@arokem

arokem Mar 29, 2015

Member

Otherwise - if it is important to know the voxel size (seems like it might be), you'll need to implement handling of voxel size into the function itself.

This comment has been minimized.

@ajnirp

ajnirp Mar 30, 2015

Contributor

voxel_size isn't really important for this function. I'll remove the kwarg in the doctest.

This comment has been minimized.

@arokem

arokem Mar 30, 2015

Member

The tests are still failing (see:
https://travis-ci.org/nipy/dipy/jobs/56414190), because of the randomness.
You might want to set the random seed (as in this question:
http://stackoverflow.com/questions/16341484/doctest-for-a-function-using-a-randomly-generated-variable),
to make sure you always get the same answer.

On Mon, Mar 30, 2015 at 1:26 AM, Rohan Prinja notifications@github.com
wrote:

In dipy/tracking/utils.py
#614 (comment):

  •    `[x, y, z]` where `[x, y, z, 1] == np.dot(affine, [i, j, k , 1])`.
    
  • See Also

  • seeds_from_mask
  • Raises

  • ValueError
  •    When `mask` is not a three-dimensional array
    
  • Examples

  • mask = np.zeros((3,3,3), 'bool')

  • mask[0,0,0] = 1

  • random_seeds_from_mask(mask, seeds_per_voxel=1, voxel_size=[1,1,1])

voxel_size isn't really important for this function. I'll remove the
kwarg in the doctest.


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

This comment has been minimized.

@ajnirp

ajnirp Mar 30, 2015

Contributor

Thank you for the link, I wasn't aware.
Hopefully the doctests will pass now.

@arokem

This comment has been minimized.

Member

arokem commented Mar 30, 2015

They're still failing, but it's getting really close. I think that the failures are now only because of formatting (white-space)

Check the travis builds by clicking on the icon next to the commit message (red 'x' for failed builds, green 'v' for successful builds, orange 'o' for builds in progress) on the PR conversation page, or go to https://travis-ci.org/nipy/dipy to find the relevant build. Also - you can run the tests with a '--with-doctest' to run the doctests as well as the unit tests.

@arokem

This comment has been minimized.

Member

arokem commented Mar 30, 2015

Yep - that seems to have done the trick.

@@ -410,12 +411,25 @@ def test_seeds_from_mask():
assert_equal(in_444.sum(), 3 * 4 * 5)
def test_random_seeds_from_mask():
mask = mask = np.random.random_integers(0, 1, size=(4, 6, 3))

This comment has been minimized.

@arokem

arokem Mar 30, 2015

Member

I think this is a typo: mask = mask = ...

@arokem

This comment has been minimized.

Member

arokem commented Apr 6, 2015

This all seems fine to me. @Garyfallidis - could you please take a look and confirm that it properly addresses the original issue?

@MrBago

This comment has been minimized.

Contributor

MrBago commented Apr 13, 2015

Looks good to me, I'll merge this soon if there are no objections.

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Apr 13, 2015

Yes Bago please take the lead on merging this. Busy week in Sherbrooke. Many thx @wenderen :)

@ajnirp

This comment has been minimized.

Contributor

ajnirp commented Apr 13, 2015

Thanks :)

@ajnirp

This comment has been minimized.

Contributor

ajnirp commented Apr 29, 2015

(ping)

arokem added a commit that referenced this pull request Apr 29, 2015

@arokem arokem merged commit 81ba3c4 into nipy:master Apr 29, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ajnirp ajnirp deleted the ajnirp:seeds-from-mask-random branch Apr 30, 2015

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