Uploaded image for project: 'Commons Math'
  1. Commons Math
  2. MATH-720

RandomDataImpl uses both JDKRandomGenerator and Well19937c, while contract/javadoc only specifies Well19937c.

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 3.0
    • Labels:

      Description

      See attached unit test. I create a distribution, sample it (not printed), set the seed to 0, and then print the next sample. I also create the same distribution again, set the seed to 0, and then print the next sample. I expect the same sample, as in both cases the seed was set to 0, just before sampling. I however get this output:

      5
      4
      

      The problem is in the org.apache.commons.math.random.RandomDataImpl class:

      • The RandomDataImpl(RandomGenerator rand) constructor states in javadoc: "@param rand the source of (non-secure) random data (may be null, resulting in default JDK-supplied generator)"
      • reSeed(long seed) method does: if (rand == null) rand = new JDKRandomGenerator();
      • reSeed() method does: if (rand == null) rand = new JDKRandomGenerator();
      • class javadoc states: "If no <code>RandomGenerator</code> is provided in the constructor, the default is to use a Well19937c generator."
      • getRan() does: if (rand == null) rand = new Well19937c(System.currentTimeMillis() + System.identityHashCode(this));
      • getRan() states in javadoc: "Creates and initializes a default generator if null. Uses a Well19937c generator with System.currentTimeMillis() + System.identityHashCode(this)) as the default seed."

      It seems that only Well19937c should be used, but the constructor javadoc, and the implementation of the reSeed methods was not updated to match this. I think the partial changes were done in MATH-701, more specifically, in commit [1197626] (and related commit [1197716]).

      1. TestRandom.java
        0.5 kB
        Dennis Hendriks

        Issue Links

          Activity

          Hide
          dhendriks Dennis Hendriks added a comment -

          TestRandom.java: shows the problem, as described in the issue description.

          Show
          dhendriks Dennis Hendriks added a comment - TestRandom.java: shows the problem, as described in the issue description.
          Hide
          erans Gilles added a comment -

          Good catch!

          This bug shows how safer it would be to have the "rand" field "final" (no need to test for "null" all over the place).

          Show
          erans Gilles added a comment - Good catch! This bug shows how safer it would be to have the "rand" field "final" (no need to test for "null" all over the place).
          Hide
          psteitz Phil Steitz added a comment -

          Fixed in r1213130.

          Show
          psteitz Phil Steitz added a comment - Fixed in r1213130.

            People

            • Assignee:
              Unassigned
              Reporter:
              dhendriks Dennis Hendriks
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development