-
Notifications
You must be signed in to change notification settings - Fork 61
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 #704 fix - randomu out of range with large poisson seed #1041
base: master
Are you sure you want to change the base?
Conversation
We do need to make very serious tests before accepting such patch, And we should not introduce bias in the initial range (where the code was tested) |
Ok thanks. Then we throw a warning to the user ? |
Thanks |
I added a new test case in test_random_poisson (in file testsuite/interactive_tests/test_random.pro). Everything seems to work well, with and without dSFMT |
for the record, dSFMT and gsl use the same algorithm, that give the same sequence of values (integers, converted to floats or doubles using the same trick as IDL does) for any same 'seed'. And this series of values are the same as IDL's. IDL previously used a different algorithm, less "random", still available with the option /RAN1 . The only drawback of dSFMT is that the (SIMD accelerated) random numbers do not arrive in the same sequence as in gsl . So the comparisons are not possible. Only the gsl-produced random numbers are strictly identical as IDL's. But dSFMT is many times faster than gsl (gsl algorithms are no-nonsense and not made for speed) I (probably unfortuately) used this same /RAN1 option not to get the old iDL values but to switch from dSFMT to gsl directly in the command line, not with the |
I need few time to reread/rerun the code of It is of higher importance for GDL not to have errors or bias in RANDOM related codes because it would affect our credibility. We did have errors two times in the past 😞 and the reason they survive is we did not anticipated wide enough tests. If we are not able to compute in a given range, we must put a Throw() ! |
I agree totally. |
Then if my understanding is right, to get the same sequence of numbers from both GDL and IDL with the same command, I should add the /RAN1 option in GDL ? Because i checked that: But with the POISSON keyword set, this is not true at all:
I should point out that the modifications I added do not affect the results below the upper limit that was found in #704 by @alaingdl, they just extend it to be a lot higher (I tested, it is around 1.5e+19 which corresponds roughly to the Max value of an unsigned long long), so in any case we should Throw() above a certain value, a thing which IDL does not do:
|
yes the sameness of sequence is only true for the basic blocks of 'ran()', integers
And for the derived floats and doubles, as the converter integer->float and double insures it. However all the derivative 'flavors' of randomness (Poisson etc) have not got the same kind of close attention and results may differ. |
I remind: tests confirmation needed, see comment #1041 (comment) |
I naively replaced some int variables with long ones, and modified two functions from the gsl library. Maybe this is too naive ?
At least it seemingly doesn't affect the computation speed