Mailing-List: contact issues-help@commons.apache.org; run by ezmlm
Precedence: bulk
Reply-To: issues@commons.apache.org
Date: Sun, 20 Dec 2015 18:07:46 +0000 (UTC)
From: "Rostislav Krasny (JIRA)" <jira@apache.org>
To: issues@commons.apache.org
Message-ID: <JIRA.12923189.1450490234000.78588.1450634866733@Atlassian.JIRA>
In-Reply-To: <JIRA.12923189.1450490234000@Atlassian.JIRA>
References: <JIRA.12923189.1450490234000@Atlassian.JIRA>
 <JIRA.12923189.1450490234775@arcas>
Subject: [jira] [Commented] (MATH-1300)
 BitsStreamGenerator#nextBytes(byte[]) is wrong
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit


    [ 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)