[
https://issues.apache.org/jira/browse/MATH-1300?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15065858#comment-15065858
]
Rostislav Krasny edited comment on MATH-1300 at 12/20/15 6:10 PM:
------------------------------------------------------------------
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 versions of the AbstractRandomGenerator need 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.
was (Author: rosti.bsd):
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)
