Re: New CPAN Module: Math::Random::MT::Auto

Hi. I had a lot of fun reading the source for this module. Here is some constructive criticism, ranging from the major to the pathologically pedantic.

INIT blocks are broken in Perl 5. Use BEGIN blocks instead. (For more on this, see ihb's and ikegami's replies to me below this node. BTW, just in case it isn't clear already, in that exchange I am on the wrong side of the argument.)
I understand the desire to make a drop-in replacement for Perl's rand (and srand), but I think it is a huge missed opportunity not to make this PRNG available as objects. PRNGs are a special kind of iterator; most, if not all, of the benefits of iterator objects apply to PRNG objects. Specifically, the possibility of having multiple independent PRNGs operating in parallel enables much greater modularization than having a single global PRNG like rand. This can be a real time saver, specially during testing and debugging.
I agree that statistical tests are essential for the this module's test suite.

I also agree with the opinion that _seed should be broken up. I would use a dispatch table, like this:

{
  # the values of the following hash are subs defined elsewhere
  my %dispatch = (
    dev_random  => \&_dev_random,
    dev_urandom => \&_dev_urandom,
    random_org  => \&_random_org,
    hotbits     => \&_hotbits,
  );

  sub _seed {
    my $none = $_[ 0 ] eq 'none';
    my @methods =  @_ if not $none;

    @seed = ();
    my $magic_num = magic_num();

    while ( @methods ) {
      my $method = shift @methods;

      my $need = $magic_num - @seed;

      if ( @methods &&
           looks_like_number( $methods[0] ) ) {
        my $n = shift @methods;
        $need = $n if $n < $need;
      }

      unless ( defined $dispatch{ $method } ) {
        push @errors, "Unsupported seeding method: $method";
        next;
      }
      push @seed, $dispatch{ $method }->( $need );
      last if @seed >= $magic_num;
    }

    push @errors, 'Full seed not acquired from sources: ' . @seed
      if @seed < $magic_num and not $none;

    push @seed, time, $$;
    $#seed = $magic_num - 1 if @seed > $magic_num;

    # Seed the PRNG
    seed( @seed );
  }
}
[download]

PRNG code often uses one or several "magic numbers", like the 624 that appears frequently in the implementation of _seed. As shown in the code given in the previous point, it is both easier to read and to maintain if one puts these into file-scoped variables or constants or accessors. (My preference is to use lexicals with corresponding getters or getter/setters, depending on the desired API.)

I am confused by this bit in import

        elsif ($sym =~ 'auto') {
            # To auto-seed or not
            $auto_seed = $sym =~ /no|!/;

        }
[download]

Don't you want $auto_seed to be set to $sym !~ /no|!/ ? If so, I would refactor that to something more like

        elsif ($sym =~ /^(no|!)?auto$) {
          $auto_seed = not defined $1;
        }
[download]

...although I must admit that I don't really understand the difference between importing 'noauto' and simply leaving 'auto' out of the import list altogether. If there is no difference, then I'd get rid of the 'noauto' option and change the fragment above to

        elsif ($sym eq 'auto') {
          # To auto-seed or not
          $auto_seed = 1;  # default value for $auto_seed should be 0
        }
[download]

It might be nice to have access (e.g. through a getter) to the user agent object used when contacting remote sources over HTTP. This would be similar to what LWP::Simple provides, but using an accessor, instead of exporting the package variable.
IMO the documentation could do a better job in explaining when someone may want to use this module over Perl's standard rand, or vice-versa.
I think one should relax strict for the smallest scope that requires it. Hence, in import I'd move the no strict 'refs' to
```
        {
           no strict 'refs';
           *{"${pkg}::$sym"} = \&$sym;
        }
[download]
```

require doesn't import; hence

            eval { require LWP::UserAgent; };
            if ($@) {
                push(@errors, "Failure loading LWP::UserAgent: $@");
                next;
            }
[download]

Actually, I prefer this idiom:

            eval { require LWP::UserAgent }
              or do {
                      push @errors,
                        "Failure loading LWP::UserAgent: $@";
                      next;
                    };
[download]

Update: In point 6, I changed a $sym =~ /^auto$/ to $sym eq 'auto'. What was I thinking?

the lowliest monk

Comment on Re: New CPAN Module: Math::Random::MT::Auto Select or Download Code

Replies are listed 'Best First'.
Re^2: New CPAN Module: Math::Random::MT::Auto by jdhedden (Deacon) on Jun 24, 2005 at 16:33 UTC
1. INIT blocks are broken in Perl 5. Use BEGIN blocks instead. I need the INIT block to initialize the PRNG after the user's seed source choices are known from the import subroutine. There's no way around it. I added a cautionary note and sample code to work around this issue if the user wants to `require` my module. 2. I understand the desire to make a drop-in replacement for Perl's rand (and srand), but I think it is a huge missed opportunity not to make this PRNG available as objects. PRNGs are a special kind of iterator; most, if not all, of the benefits of iterator objects apply to PRNG objects. Specifically, the possibility of having multiple independent PRNGs operating in parallel enables much greater modularization than having a single global PRNG like rand. This can be a real time saver, specially during testing and debugging. It's an engineering decision. I opted for speed and simplicity. Math::Random::MT has an OO interface for the purposes you describe. The only significant feature it's missing is the ability to save and restore PRNG state. UPDATE: I have now added a complete OO interface that is still faster than Math::Random::MT, and at the same time maintained the speed improvement on the standalone PRNG. Support for saving and restoring state vectors was also added. 3. I agree that statistical tests are essential for the this module's test suite. Statistical testing would flush out two possibilities: Either the algorithm is bad, or the code is incorrect. As I mentioned before, there seems to be adequate analysis of the algorithm to show that it is fine. As for the code, I compare the output of the PRNG for a known seed against known output provided by the algorithm's authors. (The test uses 2000 random number which exercises the shuffle routines through 4 cycles.) If that test passes, then the code is definitely correct. (Such a deterministic test is, in fact, much more stringent than a statistical one.) 4. I also agree with the opinion that _seed should be broken up. I would use a dispatch table... I have broken up this subroutine. I'll use a dispatch table as well. 5. PRNG code often uses one or several "magic numbers", like the 624 that appears frequently in the implementation of _seed. As shown in the code given in the previous point, it is both easier to read and to maintain if one puts these into file-scoped variables or constants or accessors. (My preference is to use lexicals with corresponding getters or getter/setters, depending on the desired API.) I put in a lexical variable. 6. I am confused by this bit in import `elsif ($sym =~ 'auto') { # To auto-seed or not $auto_seed = $sym =~ /no\|!/; }` [download] Eeek! A bug! (See my tag line. ;) Thanks. I'll squash it. 7. It might be nice to have access (e.g. through a getter) to the user agent object used when contacting remote sources over HTTP. This would be similar to what LWP::Simple provides, but using an accessor, instead of exporting the package variable. As far as I can see, there does not seem to be any need for the user to have access to the user agent object, and after a seed is acquired, there is no need to keep it around. I guess I don't understand your point. 8. IMO the documentation could do a better job in explaining when someone may want to use this module over Perl's standard rand, or vice-versa. I added a bit of this to the POD. 9. I think one should relax strict for the smallest scope that requires it. Hence, I'd move the `no strict 'refs'<code> to <code> { no strict 'refs'; *{"${pkg}::$sym"} = \&$sym; }` [download] Done. 10. require doesn't import `eval { require LWP::UserAgent; }; if ($@) { push(@errors, "Failure loading LWP::UserAgent: $@"); next; }` [download] I think you meant for me to add an import call. I will. Actually, I prefer this idiom: `eval { require LWP::UserAgent } or do { push @errors, "Failure loading LWP::UserAgent: $@"; next; };` [download] Not my style, but then again TMTOWTDI. :) Thanks for the great feedback. I'll upload my changes to CPAN soon. Remember: There's always one more bug.	[reply] [d/l] [select]

Replies are listed 'Best First'.

Re^2: New CPAN Module: Math::Random::MT::Auto
by jdhedden (Deacon) on Jun 24, 2005 at 16:33 UTC

1. INIT blocks are broken in Perl 5. Use BEGIN blocks instead.

I added a cautionary note and sample code to work around this issue if the user wants to require my module.

2. I understand the desire to make a drop-in replacement for Perl's rand (and srand), but I think it is a huge missed opportunity not to make this PRNG available as objects. PRNGs are a special kind of iterator; most, if not all, of the benefits of iterator objects apply to PRNG objects. Specifically, the possibility of having multiple independent PRNGs operating in parallel enables much greater modularization than having a single global PRNG like rand. This can be a real time saver, specially during testing and debugging.

Math::Random::MT has an OO interface for the purposes you describe. The only significant feature it's missing is the ability to save and restore PRNG state.

UPDATE: I have now added a complete OO interface that is still faster than Math::Random::MT, and at the same time maintained the speed improvement on the standalone PRNG. Support for saving and restoring state vectors was also added.

3. I agree that statistical tests are essential for the this module's test suite.

As I mentioned before, there seems to be adequate analysis of the algorithm to show that it is fine.

As for the code, I compare the output of the PRNG for a known seed against known output provided by the algorithm's authors. (The test uses 2000 random number which exercises the shuffle routines through 4 cycles.) If that test passes, then the code is definitely correct. (Such a deterministic test is, in fact, much more stringent than a statistical one.)

4. I also agree with the opinion that _seed should be broken up. I would use a dispatch table...

5. PRNG code often uses one or several "magic numbers", like the 624 that appears frequently in the implementation of _seed. As shown in the code given in the previous point, it is both easier to read and to maintain if one puts these into file-scoped variables or constants or accessors. (My preference is to use lexicals with corresponding getters or getter/setters, depending on the desired API.)

6. I am confused by this bit in import

        elsif ($sym =~ 'auto') {
            # To auto-seed or not
            $auto_seed = $sym =~ /no|!/;
        }
[download]

7. It might be nice to have access (e.g. through a getter) to the user agent object used when contacting remote sources over HTTP. This would be similar to what LWP::Simple provides, but using an accessor, instead of exporting the package variable.

8. IMO the documentation could do a better job in explaining when someone may want to use this module over Perl's standard rand, or vice-versa.

9. I think one should relax strict for the smallest scope that requires it. Hence, I'd move the
no strict 'refs'<code> to
<code>
        { no strict 'refs'; *{"${pkg}::$sym"} = \&$sym; }
[download]

10. require doesn't import

            eval { require LWP::UserAgent; };
            if ($@) {
                push(@errors, "Failure loading LWP::UserAgent: $@");
                next;
            }
[download]

Actually, I prefer this idiom:

            eval { require LWP::UserAgent }
              or do {
                      push @errors,
                        "Failure loading LWP::UserAgent: $@";
                      next;
                    };
[download]

Thanks for the great feedback. I'll upload my changes to CPAN soon.

Remember: There's always one more bug.

[reply]
[d/l]
[select]