On 12/28/15 8:08 PM, Gilles wrote: > On Mon, 28 Dec 2015 11:08:56 -0700, Phil Steitz wrote: >> The significant refactoring to eliminate the (standard) next(int) >> included in these changes has the possibility of introducing subtle >> bugs or performance issues. Please run some tests to verify that >> the same sequences are generated by the 3_X code > > IIUC our unit tests of the RNGs, this is covered.
No. Not sufficient. What you have done is changed the internal implementation of all of the Bitstream generators. I am not convinced that you have not broken anything. I will have to do the testing myself. I see no point in fiddling with the internals of this code that has had a lot of eyeballs and testing on it. I was not personally looking forward to researching the algorithms to make sure any invariants may be broken by these changes; but I am now going to have to do this. I have to ask why. Please at some point read [1], especially the sections on "Avoid Flexibility Syndrom" and "Value Laziness as a Virtue." Gratuitous refactoring drains community energy. > >> and the refactored >> code and benchmarks to show there is no loss in performance. > > Given that there are exactly two operations _less_ (a subtraction > and a shift), it would be surprising. > >> It >> would also be good to have some additional review of this code by >> PRNG experts. > > The "nextInt()" code is exactly the same as the "next(int)" modulo > the little change above (in the last line of the "nextInt/next" > code). > > That change in "nextInt/next" implied similarly tiny recodings in > the generic methods "nextDouble()", "nextBoolean()", ... which, apart > from that, were copied from "BitsStreamGenerator". > > [However tiny a change, I had made a mistake... and dozens of tests > started to fail. Found the typo and all was quiet again...] > > About "next(int)" being standard, it would be interesting to know > what that means. Have a look at the source code for the JDK generators, for example. > As I indicated quite clearly in one of my first posts about this > refactoring > 1. all the CM implementations generate random bits in batches > of 32 bits, and > 2. before returning, the "next(int bits)" method was truncating > the generated "int": > return x >>> (32 - bits); > > In all implementations, that was the only place where the "bits" > parameter was used, from which I concluded that the randomness > provider does not care if the request was to create less than 32 > random bits. > Taking "nextBoolean()" for example, it looks like a waste of 31 > bits (or am I missing something?). Quite possibly, yes, you are missing something. > > Of course, if some implementation were able to store the bits not > requested by the last call to "next(int)", then I'd understand that > we must really provide access to a "next(int)" method. > > Perhaps that the overhead of such bookkeeping is why the practical > algorithms chose to store integers rather than bits (?). > > As you dismissed my request about CM being able to care for a RNG > implementation based on a "long", I don't quite understand the > caring for a "next(int)" that serves no more purpose (as of current > CM). > This change is > > Gilles > > >> Phil >> >> On 12/28/15 10:23 AM, er...@apache.org wrote: >>> Repository: commons-math >>> Updated Branches: >>> refs/heads/master 7b62d0155 -> 81585a3c4 >>> >>> >>> MATH-1307 >>> >>> New base class for RNG implementations. >>> The source of randomness is provided through the "nextInt()" >>> method (to be defined in subclasses). >>> >>> >>> [...] > [1] http://www.apachecon.com/eu2007/materials/ac2006.2.pdf > > --------------------------------------------------------------------- > To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org > For additional commands, e-mail: dev-h...@commons.apache.org > > --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
