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

NF - random seeds from mask #760

Merged
merged 9 commits into from Jan 3, 2016

Conversation

Projects
None yet
4 participants
@gabknight
Contributor

gabknight commented Nov 3, 2015

  • This PR adds the fct random_seeds_from_mask_total to generate N seeds given a mask. This complements the fct random_seeds_from_mask_per_voxel that already generates seeds given a mask. This is useful when you want N streamlines per region, with regions of various size.
  • It also fixes a small bug. If N seeds were sent to LocalTracking with return_all==True, the returned streamlines excluded all seeds with no valid initial direction. Now, if a seed has no valid initial direction, it return a streamline containing only the seed position.

@gabknight gabknight changed the title from NF - random seeds from mask total to NF - random seeds from mask Nov 4, 2015

@arokem

This comment has been minimized.

Member

arokem commented Nov 8, 2015

These function names are really long. Might it make more sense to incorporate the new functionality into the existing function as a kwarg set per default to the 'old' functionality ('voxel'), but allowing use of the new functionality ('total')?

@gabknight

This comment has been minimized.

Contributor

gabknight commented Nov 19, 2015

Thx @arokem. I did what you suggested. let me know what you think.

@arokem

This comment has been minimized.

Member

arokem commented Nov 19, 2015

Great! Just one more suggestion - to test the BF that you had there. It seems that we'd want a test to prevent that from creeping back into the code.

@@ -503,6 +515,11 @@ def random_seeds_from_mask(mask, seeds_per_voxel=1, affine=None):
seeds = where + grid - .5
seeds = asarray(seeds)
if not seed_count_per_voxel:
# Randomize the seeds and select the requested amount
np.random.shuffle(seeds)

This comment has been minimized.

@MarcCote

MarcCote Nov 24, 2015

Contributor

Wouldn't be better to create its own random generator using a seed (ideally provided as an argument to this function) instead of relying on the user having to call np.random.seed(seed). I think changing the seed of the global random generator (i.e. the one in np.random) is prone unreproducible results. (e.g., for some reason you decided to call np.random.rand() in another part of your code before calling this function).

This comment has been minimized.

@gabknight

gabknight Dec 8, 2015

Contributor

I see your point. Currently there is only one dipy function having a random_seed parameter (dipy.align.parzenhist.sample_domain_regular). Elsewhere, it is up to the user to set random.seed before calling functions using a random number generator.

If we add a random seed parameter to functions, the random seed parameter should be uniform throughout the code, and adding a parameter seed=0 as in dipy.align.parzenhist.sample_domain_regular could bring a bit a confusion for this particular function.

What do you guys think? Should we set a standard parameter name to add to function using np.random, say random_seed=None (so we don't change the state if None)?

This comment has been minimized.

@Garyfallidis

Garyfallidis Dec 30, 2015

Member

My view is that there is no reason to have a parameter for the random state. It will bring more confusion than good. But if you add in the documentation that you can actually specify the randomness (using np.random.seed...) before calling the function that would be enough.

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Dec 30, 2015

@gabknight can you rebase this if needed and you think is ready? And I will merge it asap.

@gabknight gabknight force-pushed the gabknight:NF_random_seeds_from_mask_total branch from 344f188 to 4528672 Dec 31, 2015

@gabknight

This comment has been minimized.

Contributor

gabknight commented Jan 3, 2016

Done. @Garyfallidis, all good on my side

distributing the seeds, it randomly places the seeds within the voxels
specified by the ``mask``
specified by the ``mask``. The initial random conditions can be set using
``numpy.random.seed(...)``, prior to call this function.

This comment has been minimized.

@arokem

arokem Jan 3, 2016

Member

=> "prior to calling this function"

@arokem

This comment has been minimized.

Member

arokem commented Jan 3, 2016

Just small comment on docs. Otherwise - I am +1 for the merge.

@gabknight

This comment has been minimized.

Contributor

gabknight commented Jan 3, 2016

Thanks!

Garyfallidis added a commit that referenced this pull request Jan 3, 2016

@Garyfallidis Garyfallidis merged commit b5c18db into nipy:master Jan 3, 2016

1 check passed

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

This comment has been minimized.

Member

Garyfallidis commented Jan 3, 2016

Good job! Let's move to the next frontier! 👍

@gabknight gabknight deleted the gabknight:NF_random_seeds_from_mask_total branch Aug 29, 2017

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