commons-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Rostislav Krasny (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (MATH-1300) BitsStreamGenerator#nextBytes(byte[]) is wrong
Date Sun, 20 Dec 2015 18:07:46 GMT

    [ https://issues.apache.org/jira/browse/MATH-1300?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15065858#comment-15065858
] 

Rostislav Krasny commented on MATH-1300:
----------------------------------------

WELL generators in CM inherite the same nextBytes() of BitsStreamGenerator. They extend an
AbstractWell that extends the BitsStreamGenerator class. The discussed issue of the BitsStreamGenerator#nextBytes()
relates to all BitsStreamGenerator descendants. I used the MersenneTwister class just for
the demonstration.

After looking at the Gilles commit I've a few questions:
https://git1-us-west.apache.org/repos/asf?p=commons-math.git;a=commitdiff;h=1d635088f697178660b6e1c9a89d2b7d3bbe2d29

1. Why did you do the change so minimal with many unneded operations still in the code instead
of taking the code I've proposed? I'm talking about the unneded & 0xff operations, not
optimal index incrementation and cases where the last shift right 8 bits isn't needed. If
you think my code is hard to understand you may add comments into it. In my opinion this is
very simple code. Anyway I think the performance is important.

2. I've just noticed the AbstractRandomGenerator has its own implementation of the nextBytes()
method. Why does it need a differently implemented nextBytes()? And why that implementation
is so strange? After Gilles commit it's even stranger.

before commit:
{code:java}
     @Override
     public void nextBytes(byte[] bytes) {
         int bytesOut = 0;
         while (bytesOut < bytes.length) {
           int randInt = nextInt();
           for (int i = 0; i < 3; i++) {
               if ( i > 0) {
                  randInt >>= 8;
               }
               bytes[bytesOut++] = (byte) randInt;
               if (bytesOut == bytes.length) {
                   return;
               }
           }
         }
     }
{code}
after commit:
{code:java}
     @Override
     public void nextBytes(byte[] bytes) {
         int bytesOut = 0;
         while (bytesOut < bytes.length) {
             int randInt = nextInt();
             for (int i = 0; i < 3; i++) {
                 if (i > 0) {
                     randInt >>= 8;
                 }
             }
             if (bytesOut < bytes.length) {
                 bytes[bytesOut++] = (byte) randInt;
                 if (bytesOut == bytes.length) {
                     return;
                 }
             }
         }
     }
{code}
The original version before commit is not optimized but this is not the only issue. It uses
only three bytes of the random int, doesn't it? And after Gilles commit it uses only one byte
of the random int, making many unneeded actions around. Both version of AbstractRandomGenerator
needs more calls to nextInt() than java.util.Random. Both versions look as a bug.

In my opinion both the BitsStreamGenerator and the AbstractRandomGenerator should use the
same nextBytes() code that I proposed above.

> BitsStreamGenerator#nextBytes(byte[]) is wrong
> ----------------------------------------------
>
>                 Key: MATH-1300
>                 URL: https://issues.apache.org/jira/browse/MATH-1300
>             Project: Commons Math
>          Issue Type: Bug
>    Affects Versions: 3.5
>            Reporter: Rostislav Krasny
>         Attachments: MersenneTwister2.java, TestMersenneTwister.java
>
>
> Sequential calls to the BitsStreamGenerator#nextBytes(byte[]) must generate the same
sequence of bytes, no matter by chunks of which size it was divided. This is also how java.util.Random#nextBytes(byte[])
works.
> When nextBytes(byte[]) is called with a bytes array of length multiple of 4 it makes
one unneeded call to next(int) method. This is wrong and produces an inconsistent behavior
of classes like MersenneTwister.
> I made a new implementation of the BitsStreamGenerator#nextBytes(byte[]) see attached
code.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Mime
View raw message