[
https://issues.apache.org/jira/browse/MATH-1300?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15065919#comment-15065919
]
Gilles commented on MATH-1300:
------------------------------
bq. 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.
Sure.
My comment about deprecating the {{MersenneTwister}} class was not meant to
imply that the redundant call should not be removed.
bq. 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?
Because we generally prefer one commit per issue.
This issue (MATH-1300) was about a redundant call that prevented the property
which you expected.
Fixing that first does not prevent further changes.
bq. I'm talking about the unneded & 0xff operations,
I'm going to do that in another commit.
Usually this should also require another report, but I'll just refer to the
discussion here in the commit message.
bq. not optimal index incrementation and cases where the last shift right 8
bits isn't needed.
See above discussion.
Please open another issue, as it is not related to your original statement
(IMHO: "wrong != suboptimal").
bq. If you think my code is hard to understand you may add comments into it. In
my opinion this is very simple code.
The "for" statement in your code is not easy to understand (for a "for"
statement, that is).
If I'm not mistaken, a CM unwritten rule for code is "No hard-coded numbers".
So I would not just commit your code as is.
bq. Anyway I think the performance is important.
Agreed, just not at all cost, IMO.
Maintenance is also a parameter to take into account, especially if the gain of
less clear code is quite small (and subject to erratic variations between HW
and JVM).
This does not mean that I prefer to leave it at that; just it should also be
another issue.
bq. 2. I've just noticed the AbstractRandomGenerator has its own implementation
of the nextBytes() method. Why does it need a differently implemented
nextBytes()?
It doesn't; I just grouped it in the same commit because it is the same
problem, to be fixed in the most obvious way before further changing the code
(another CM rule).
bq. And why that implementation is so strange? After Gilles commit it's even
stranger.
Sorry, my mistake; I'll fix ASAP. Thanks for the review. I hope I'll get it
right in the next commit.
bq. In my opinion both the BitsStreamGenerator and the AbstractRandomGenerator
should use the same nextBytes() code
Agreed.
But they are not in the same hierarchy. It's a pity. If it should be the same
code, then the code should be shared. Suggestions on how to achieve that are
welcome.
> 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)
