Random elements of AA aren't random

[orlitzky]

The AA.random_element() method only ever returns integers
among -2, -1, 0, 1, 2:

sage: ' '.join(str(AA.random_element()) for i in range(100))
2 0 0 2 1 2 -1 0 -2 -2 -2 0 -2 0 2 -1 1 0 0 1 1 -1 2 -1 0 2 1
0 0 2 1 2 0 -1 -2 -2 -1 2 -2 1 -1 -2 -2 2 -1 0 1 -1 2 2 0 -2
-1 -1 1 -2 2 0 2 -2 0 -2 1 -2 0 -2 -2 -2 -1 -2 -1 2 0 0 1 0 0
-2 0 -1 2 -2 0 -2 2 -1 -1 1 1 -2 -1 0 -2 -2 -1 2 -2 -1 -2 -2

That's a bit un-random, even by Sage standards.
A simple improvement would be to return the real part
of QQbar.random_element():

sage: [QQbar.random_element().real() for i in range(5) ]
[-5.372281323269015?,
 -0.2000000000000000?,
 0.50000000000000000?,
 -0.50000000000000000?,
 0.1906845620110298?]

Component: algebra

Author: Ben Livingston, Michael Orlitzky

Branch/Commit: 94c07ba

Reviewer: Lorenz Panny

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

[bmlivin]

comment:1

What's happening is that the class aliased to AA, sage.rings.qqbar:AlgebraicRealField, doesn't have a random_element method, which is causing its base class's (sage.rings:Ring's) random_element to be called. I'm going to look into why this is happening.

[bmlivin]

Branch: u/gh-bmlivin/random_elements_of_aa_aren_t_random

[bmlivin]

New commits:

4772419

Added random_element method for sage.rings.qqbar.AlgebraicRealField which returns the real part of sage.rings.qqbar.AlgebraicField::random_element and moved sage.rings.qqbar.AlgebraicField::random_element to its parent class so it can be called by sage.rings.qqbar.AlgebraicRealField

[bmlivin]

Author: Ben Livingston

[bmlivin]

Commit: 4772419

[orlitzky]

comment:4

This works, but I'm curious why you wanted to use a superclass method instead of

def random_element(self, poly_degree=2, *args, **kwds):
    r"""
    ...
    """
    return QQbar.random_element(poly_degree, *args, **kwds).real()

Either way works, but your version moves some things into the "common" class that aren't common to the two fields (they only work for QQbar, and have to be overridden for AA). I didn't write these classes, but that seems to go against the design; e.g. nothing else in the "common" class is overridden in the subclasses.

[fchapoton]

comment:5

patchbot tells you that the added doctest should start with r"""

[mkoeppe]

comment:6

Setting new milestone based on a cursory review of ticket status, priority, and last modification date.

[mkoeppe]

comment:7

Setting a new milestone for this ticket based on a cursory review.