Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SL2Z.random_element unstable, ZZ.random_element does not ignore bounds not needed for distribution #32124

Closed
kliem opened this issue Jul 4, 2021 · 11 comments

Comments

@kliem
Copy link
Contributor

kliem commented Jul 4, 2021

sage: for _ in range(1000): _ = SL2Z.random_element(5, distribution='1/n')
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-18-454018fc8ca8> in <module>
----> 1 for _ in range(Integer(1000)): _ = SL2Z.random_element(Integer(5), distribution='1/n')

~/Applications/sage/local/lib/python3.8/site-packages/sage/modular/arithgroup/congroup_sl2z.py in random_element(self, bound, *args, **kwds)
    212         d = ZZ.random_element(1-bound, bound, *args, **kwds)
    213         if gcd(c,d) != 1: # try again
--> 214             return self.random_element(bound, *args, **kwds)
    215         else:
    216             a,b,c,d = lift_to_sl2z(c,d,0)

~/Applications/sage/local/lib/python3.8/site-packages/sage/modular/arithgroup/congroup_sl2z.py in random_element(self, bound, *args, **kwds)
    231                 wlo = min(wlo, ((bound - b)/ZZ(-d)).ceil())
    232 
--> 233             w = ZZ.random_element(1-wlo, whi, *args, **kwds)
    234             a += c*w
    235             b += d*w

~/Applications/sage/local/lib/python3.8/site-packages/sage/rings/integer_ring.pyx in sage.rings.integer_ring.IntegerRing_class.random_element (build/cythonized/sage/rings/integer_ring.c:6515)()
    715             raise TypeError("x must be > 0")
    716         if x is not None and y is not None and x >= y:
--> 717             raise TypeError("x must be < y")
    718         self._randomize_mpz(z.value, x, y, distribution)
    719         return z

TypeError: x must be < y
sage: ZZ.random_element(-10, -5, distribution="mpz_rrandomb")                                                                                                                       
---------------------------------------------------------------------------
OverflowError                             Traceback (most recent call last)
<ipython-input-1-6e917954ddfe> in <module>
----> 1 ZZ.random_element(-Integer(10), -Integer(5), distribution="mpz_rrandomb")

/srv/public/kliem/sage/local/lib/python3.7/site-packages/sage/rings/integer_ring.pyx in sage.rings.integer_ring.IntegerRing_class.random_element (build/cythonized/sage/rings/integer_ring.c:6533)()
    733         if x is not None and y is not None and x >= y:
    734             raise TypeError("x must be < y")
--> 735         self._randomize_mpz(z.value, x, y, distribution)
    736         return z
    737 

/srv/public/kliem/sage/local/lib/python3.7/site-packages/sage/rings/integer_ring.pyx in sage.rings.integer_ring.IntegerRing_class._randomize_mpz (build/cythonized/sage/rings/integer_ring.c:7036)()
    778             if x is None:
    779                 raise ValueError("must specify x to use 'distribution=mpz_rrandomb'")
--> 780             mpz_rrandomb(value, rstate.gmp_state, int(x))
    781         elif distribution == "gaussian":
    782             global _prev_discrete_gaussian_integer_sampler

OverflowError: can't convert negative value to unsigned long

Component: modular forms

Author: Jonathan Kliem

Branch/Commit: e6a697a

Reviewer: Frédéric Chapoton

Issue created by migration from https://trac.sagemath.org/ticket/32124

@kliem kliem added this to the sage-9.4 milestone Jul 4, 2021
@kliem
Copy link
Contributor Author

kliem commented Jul 4, 2021

comment:1

I would say the problem is in ZZ.random_element. It claims that x and y are ignored, if the distribution is '1/n'. However, it still raises an error if y >= x, which is not what I would call ignoring x and y.

@kliem

This comment has been minimized.

@kliem kliem changed the title SL2Z.random_element unstable SL2Z.random_element unstable, ZZ.random_element does not ignore bounds not needed for distribution Jul 4, 2021
@kliem
Copy link
Contributor Author

kliem commented Jul 4, 2021

New commits:

e6a697atruely ignore ignored bounds for ZZ.random_element

@kliem
Copy link
Contributor Author

kliem commented Jul 4, 2021

Author: Jonathan Kliem

@kliem
Copy link
Contributor Author

kliem commented Jul 4, 2021

Branch: public/32124

@kliem
Copy link
Contributor Author

kliem commented Jul 4, 2021

Commit: e6a697a

@dimpase
Copy link
Member

dimpase commented Jul 4, 2021

comment:4

good catch!

@fchapoton
Copy link
Contributor

comment:5

ok, let it be

@fchapoton
Copy link
Contributor

Reviewer: Frédéric Chapoton

@kliem
Copy link
Contributor Author

kliem commented Jul 6, 2021

comment:6

Thank you.

@vbraun
Copy link
Member

vbraun commented Jul 23, 2021

Changed branch from public/32124 to e6a697a

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants