Opened 12 years ago

Closed 10 years ago

Last modified 7 years ago

#9770 closed defect (fixed)

SphericalDistribution() is not random

Reported by: schilly Owned by: amhou
Priority: major Milestone: sage-5.0
Component: statistics Keywords:
Cc: Merged in: sage-5.0.beta9
Authors: Douglas McNeil Reviewers: Jason Grout, Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #9958 Stopgaps:

Status badges

Description (last modified by chapoton)

In the following list l, some elements repeat quite often:

sage: l = [ SphericalDistribution(dimension=2).get_random_element() for _ in range(1000)]
sage: uniq = []
sage: for x in l:
    if x not in uniq:
        uniq.append(x)
....:
sage: len(uniq)
34

The output is not random. For example, the first line is repeated ~30 times in the 1000 lines of output. It works fine if SphericalDistribution is only instantiated once!

Attachments (4)

SageDays_random_element_bug.sws (1.7 KB) - added by jreaton 12 years ago.
trac_9770_fix_distribution_seeds.patch (2.7 KB) - added by dsm 11 years ago.
fix seed randomization
trac_9770_fix_distribution_seeds_v2.patch (3.0 KB) - added by dsm 10 years ago.
hopefully maxint-safe version
trac_9770_fix_distribution_seeds_v4.patch (3.4 KB) - added by dsm 10 years ago.
rebased to 5.0.beta7

Download all attachments as: .zip

Change History (23)

Changed 12 years ago by jreaton

comment:1 Changed 12 years ago by jreaton

The bug is not unique to the spherical distribution; rather, it has to do with whether the distribution is instantiated prior to the use of the random element method. The worksheet above illustrates the same behavior with the Gaussian and uniform distributions.

comment:2 Changed 11 years ago by dsm

  • Status changed from new to needs_review

Oh, wow. This is because of the lines

            self.seed = random.randint(1, 2^32)

2 xor 32 is 34.. and the ^ vs. ** bug strikes again.

Last edited 10 years ago by dsm (previous) (diff)

Changed 11 years ago by dsm

fix seed randomization

comment:3 Changed 11 years ago by jason

  • Reviewers set to Jason Grout
  • Status changed from needs_review to positive_review

Wow, good catch. Affected file passes tests; code looks good.

Can you fill in the author name?

comment:4 Changed 11 years ago by schilly

  • Authors set to Douglas McNeil

comment:5 Changed 11 years ago by jdemeyer

  • Merged in set to sage-4.8.alpha5
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:6 Changed 11 years ago by jdemeyer

  • Merged in sage-4.8.alpha5 deleted
  • Resolution fixed deleted
  • Status changed from closed to new

On hawk (OpenSolaris? 06.2009-32):

sage -t -long  -force_lib devel/sage/sage/gsl/probability_distribution.pyx
**********************************************************************
File "/export/home/buildbot/build/sage/hawk-1/hawk_full/build/sage-4.8.alpha5/devel/sage-main/sage/gsl/probability_distribution.pyx", line 339:
    sage: T = RealDistribution('rayleigh', sigma)
Exception raised:
    Traceback (most recent call last):
      File "/export/home/buildbot/build/sage/hawk-1/hawk_full/build/sage-4.8.alpha5/local/bin/ncadoctest.py", line 1231, in run_one_test
        self.run_one_example(test, example, filename, compileflags)
      File "/export/home/buildbot/build/sage/hawk-1/hawk_full/build/sage-4.8.alpha5/local/bin/sagedoctest.py", line 38, in run_one_example
        OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags)
      File "/export/home/buildbot/build/sage/hawk-1/hawk_full/build/sage-4.8.alpha5/local/bin/ncadoctest.py", line 1172, in run_one_example
        compileflags, 1) in test.globs
      File "<doctest __main__.example_12[16]>", line 1, in <module>
        T = RealDistribution('rayleigh', sigma)###line 339:
    sage: T = RealDistribution('rayleigh', sigma)
      File "probability_distribution.pyx", line 503, in sage.gsl.probability_distribution.RealDistribution.__init__ (sage/gsl/probability_distribution.c:2309)
        self.seed = random.randint(1, 2**32)
    OverflowError: long int too large to convert to int
**********************************************************************
[...many more like this...]

sage -t -long  -force_lib devel/sage/sage/matrix/constructor.py
**********************************************************************
File "/export/home/buildbot/build/sage/hawk-1/hawk_full/build/sage-4.8.alpha5/devel/sage-main/sage/matrix/constructor.py", line 2944:
    sage: print "ignore this";  B=random_matrix(FiniteField(7), 4, 4, algorithm='echelon_form', num_pivots=3); B # random
Exception raised:
    Traceback (most recent call last):
      File "/export/home/buildbot/build/sage/hawk-1/hawk_full/build/sage-4.8.alpha5/local/bin/ncadoctest.py", line 1231, in run_one_test
        self.run_one_example(test, example, filename, compileflags)
      File "/export/home/buildbot/build/sage/hawk-1/hawk_full/build/sage-4.8.alpha5/local/bin/sagedoctest.py", line 38, in run_one_example
        OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags)
      File "/export/home/buildbot/build/sage/hawk-1/hawk_full/build/sage-4.8.alpha5/local/bin/ncadoctest.py", line 1172, in run_one_example
        compileflags, 1) in test.globs
      File "<doctest __main__.example_19[10]>", line 1, in <module>
        print "ignore this";  B=random_matrix(FiniteField(Integer(7)), Integer(4), Integer(4), algorithm='echelon_form', num_pivots=Integer(3)); B # random###line 2944:
    sage: print "ignore this";  B=random_matrix(FiniteField(7), 4, 4, algorithm='echelon_form', num_pivots=3); B # random
      File "/export/home/buildbot/build/sage/hawk-1/hawk_full/build/sage-4.8.alpha5/local/lib/python/site-packages/sage/matrix/constructor.py", line 1250, in random_matrix
        return random_rref_matrix(parent, *args, **kwds)
      File "/export/home/buildbot/build/sage/hawk-1/hawk_full/build/sage-4.8.alpha5/local/lib/python/site-packages/sage/matrix/constructor.py", line 3020, in random_rref_matrix
        pivot_generator=pd.RealDistribution("beta",[1.6,4.3])
      File "probability_distribution.pyx", line 503, in sage.gsl.probability_distribution.RealDistribution.__init__ (sage/gsl/probability_distribution.c:2309)
    OverflowError: long int too large to convert to int
**********************************************************************
[...]

comment:7 Changed 11 years ago by dsm

  • Status changed from new to needs_info

Urf. Probably (?) we can simply replace 2**32 here with sys.maxint, but I can't be sure because I can't reproduce.

Emailed a guy who I know has access to a solaris box :-) but haven't heard back. If anyone with an account on hawk could report the results of a cut-and-paste of the following

preparser(False)
import random, sys

nn = [2**31, 2**32, sys.maxint]
for n in nn:
    for d in -1, 0, 1:
        print n, d, n+d, repr(n+d), type(n+d)
        try:
            seed = random.randint(1, n+d)
        except Exception as err:
            print err
        try:
            random.seed(n+d)
        except Exception as err:
            print err

from within Sage, that would test whether I understand what's going on.

Last edited 10 years ago by dsm (previous) (diff)

comment:8 Changed 11 years ago by jdemeyer

Output of your Sage script on hawk:

2147483648 -1 2147483647 2147483647L <type 'long'>
2147483648 0 2147483648 2147483648L <type 'long'>
2147483648 1 2147483649 2147483649L <type 'long'>
4294967296 -1 4294967295 4294967295L <type 'long'>
4294967296 0 4294967296 4294967296L <type 'long'>
4294967296 1 4294967297 4294967297L <type 'long'>
2147483647 -1 2147483646 2147483646 <type 'int'>
2147483647 0 2147483647 2147483647 <type 'int'>
2147483647 1 2147483648 2147483648L <type 'long'>

comment:9 Changed 10 years ago by jdemeyer

* bump *

Changed 10 years ago by dsm

hopefully maxint-safe version

comment:10 Changed 10 years ago by dsm

  • Status changed from needs_info to needs_review

Version modified to (hopefully) avoid overflow errors without sacrificing entropy. @jdemeyer, you mind trying it on hawk?

comment:11 Changed 10 years ago by davidloeffler

Apply trac_9770_fix_distribution_seeds_v2.patch

(for the patchbot, which is trying to install both patches at once)

comment:12 Changed 10 years ago by davidloeffler

  • Dependencies set to #9958
  • Reviewers changed from Jason Grout to Jason Grout, PatchBot
  • Status changed from needs_review to needs_work
  • Work issues set to needs rebase

This seems to conflict (in a rather trivial way) with #9958, and hence doesn't apply to the latest Sage beta.

Changed 10 years ago by dsm

rebased to 5.0.beta7

comment:13 Changed 10 years ago by dsm

  • Status changed from needs_work to needs_review

comment:14 Changed 10 years ago by davidloeffler

Apply trac_9770_fix_distribution_seeds_v4.patch

(for patchbot)

comment:15 Changed 10 years ago by jdemeyer

  • Work issues needs rebase deleted

comment:16 Changed 10 years ago by jdemeyer

  • Reviewers changed from Jason Grout, PatchBot to Jason Grout

Let's not anthropomorphize the PatchBot? :-)

comment:17 Changed 10 years ago by jdemeyer

  • Reviewers changed from Jason Grout to Jason Grout, Jeroen Demeyer
  • Status changed from needs_review to positive_review

Seems to work as it should...

comment:18 Changed 10 years ago by jdemeyer

  • Merged in set to sage-5.0.beta9
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:19 Changed 7 years ago by chapoton

  • Description modified (diff)
Note: See TracTickets for help on using tickets.