Opened 7 years ago

Closed 6 years ago

Last modified 2 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:

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 7 years ago.
trac_9770_fix_distribution_seeds.patch (2.7 KB) - added by dsm 6 years ago.
fix seed randomization
trac_9770_fix_distribution_seeds_v2.patch (3.0 KB) - added by dsm 6 years ago.
hopefully maxint-safe version
trac_9770_fix_distribution_seeds_v4.patch (3.4 KB) - added by dsm 6 years ago.
rebased to 5.0.beta7

Download all attachments as: .zip

Change History (23)

Changed 7 years ago by jreaton

comment:1 Changed 7 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 6 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 6 years ago by dsm (previous) (diff)

Changed 6 years ago by dsm

fix seed randomization

comment:3 Changed 6 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 6 years ago by schilly

  • Authors set to Douglas McNeil

comment:5 Changed 6 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 6 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 6 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 6 years ago by dsm (previous) (diff)

comment:8 Changed 6 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 6 years ago by jdemeyer

* bump *

Changed 6 years ago by dsm

hopefully maxint-safe version

comment:10 Changed 6 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 6 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 6 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 6 years ago by dsm

rebased to 5.0.beta7

comment:13 Changed 6 years ago by dsm

  • Status changed from needs_work to needs_review

comment:14 Changed 6 years ago by davidloeffler

Apply trac_9770_fix_distribution_seeds_v4.patch

(for patchbot)

comment:15 Changed 6 years ago by jdemeyer

  • Work issues needs rebase deleted

comment:16 Changed 6 years ago by jdemeyer

  • Reviewers changed from Jason Grout, PatchBot to Jason Grout

Let's not anthropomorphize the PatchBot? :-)

comment:17 Changed 6 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 6 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 2 years ago by chapoton

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