BF - fixed random generator seed value too large to convert to int error
#2892
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #2892 +/- ##
==========================================
+ Coverage 81.40% 81.71% +0.30%
==========================================
Files 145 146 +1
Lines 20145 20347 +202
Branches 3217 3226 +9
==========================================
+ Hits 16400 16627 +227
+ Misses 2929 2902 -27
- Partials 816 818 +2
|
gabknight
changed the title
value too large to convert to int error
There was a problem hiding this comment.
Hi @gabknight,
Thanks for fixing this. Overall, it looks good to me. See below my question and then, it is ready to merge. Thanks
| seeds=seeds, | ||
| affine=np.eye(4), | ||
| step_size=1., | ||
| random_seed=rdm_seed)) |
There was a problem hiding this comment.
I would add a small test to make sure it fails if you put a random seed = np.iinfo(np.int32).max + 1.
Actually, What is error when this happens (I know, it should not)
There was a problem hiding this comment.
random_seed is not directly input to the random generator. It is a hash of random_seed and the tractography seed position:
s_random_seed = hash(np.abs((np.sum(s)) + self.random_seed)) \
% (2**32 - 1)
random.seed(s_random_seed)
np.random.seed(s_random_seed)
fast_numpy.seed(s_random_seed)
It is a OverflowError. I added a test in fast_numpy.
There was a problem hiding this comment.
What I mean is: if I do naively that:
randoms_seeds = np.iinfo(np.uint32).max + 1 #. or -1
_ = Streamlines(LocalTracking(direction_getter=dg,
stopping_criterion=sc,
seeds=seeds,
affine=np.eye(4),
step_size=1.,
random_seed=randoms_seeds))I get this error:
Traceback (most recent call last):
File "/Users/skoudoro/devel/dipy/dipy/tracking/tests/test_tracking.py", line 988, in <module>
test_random_seed_initialization()
File "/Users/skoudoro/devel/dipy/dipy/tracking/tests/test_tracking.py", line 981, in test_random_seed_initialization
_ = Streamlines(LocalTracking(direction_getter=dg,
File "/opt/homebrew/Caskroom/miniforge/base/envs/dipy-39-x86/lib/python3.9/site-packages/nibabel/streamlines/array_sequence.py", line 148, in __init__
self.extend(iterable)
File "/opt/homebrew/Caskroom/miniforge/base/envs/dipy-39-x86/lib/python3.9/site-packages/nibabel/streamlines/array_sequence.py", line 324, in extend
for e in elements:
File "/opt/homebrew/Caskroom/miniforge/base/envs/dipy-39-x86/lib/python3.9/site-packages/dipy/tracking/utils.py", line 886, in transform_tracking_output
for sl in streamlines:
File "/opt/homebrew/Caskroom/miniforge/base/envs/dipy-39-x86/lib/python3.9/site-packages/dipy/tracking/local_tracking.py", line 140, in _generate_tractogram
fast_numpy.seed(s_random_seed)
File "fast_numpy.pyx", line 188, in dipy.utils.fast_numpy.seed
OverflowError: value too large to convert to intI feel it is easy to prevent, via a small check in the LocalTracking __init__
There was a problem hiding this comment.
not all users will understand this OverflowError: value too large to convert to int
There was a problem hiding this comment.
@skoudoro I could not reproduce your error. If the random_seed input to local_tracking is higher than 2^{32}, it will be used in the hash in dipy.tracking.local_tracking.py line 136, but then reduce to a value < 2^{32} by the modulo line 137. OverflowError should not happen when calling LocalTracking.
Could you try your code on the updated PR?
|
Thank you @gabknight ! Tested and all good |
fast_numpy.seed(int s)could raise an error if the input value was too high for anint. I changed the input tofast_numpy.seed(cnp.npy_uint32 s)as it should have been and added a new test.Thanks @EmmaRenauld for reporting this.