Improve FiniteField_givaroElement

src/sage/combinat/designs/difference_family.py

@@ -802,8 +802,7 @@ def one_radical_difference_family(K, k):
     # instead of the complicated multiplicative group K^*/(±C) we use the
     # discrete logarithm to convert everything into the additive group Z/cZ
     c = m * (q-1) // e  # cardinal of ±C
-    from sage.groups.generic import discrete_log
-    logA = [discrete_log(a,x) % c for a in A]
+    logA = [a.log(x) % c for a in A]
 
     # if two elements of A are equal modulo c then no tiling is possible
     if len(set(logA)) != m:

src/sage/rings/finite_rings/element_base.pyx

@@ -723,7 +723,7 @@ cdef class FinitePolyExtElement(FiniteRingElement):
             sage: S(0).multiplicative_order()
             Traceback (most recent call last):
             ...
-            ArithmeticError: Multiplicative order of 0 not defined.
+            ArithmeticError: multiplicative order of 0 not defined
         """
         if self.is_zero():
             raise ArithmeticError("Multiplicative order of 0 not defined.")

src/sage/rings/finite_rings/element_givaro.pyx

@@ -66,6 +66,8 @@ from cypari2.stack cimport clear_stack
 
 from sage.structure.parent cimport Parent
 from sage.structure.element cimport Vector
+from sage.rings.fast_arith cimport arith_int
+cdef arith_int ai = arith_int()
 
 from sage.interfaces.abc import GapElement
 
@@ -837,6 +839,19 @@ cdef class FiniteField_givaro_iterator:
 cdef class FiniteField_givaroElement(FinitePolyExtElement):
     """
     An element of a (Givaro) finite field.
+
+    Internal implementation detail: ``self.element`` is a ``cdef int`` member such that:
+
+    - if ``self.element == 0``, then ``self.is_zero()``,
+
+    - otherwise, ``self == g^self.element`` where ``g`` is a multiplicative generator.
+
+    In Givaro code, the type of ``element`` is known by the typename ``Rep``.
+
+    It is preferred to use the exposed interface of Givaro than to rely on this implementation detail.
+
+    The C function :func:`make_FiniteField_givaroElement` can be internally used to construct a
+    :func:`FiniteField_givaroElement` object given ``int element``.
     """
 
     def __init__(FiniteField_givaroElement self, parent):
@@ -1036,7 +1051,7 @@ cdef class FiniteField_givaroElement(FinitePolyExtElement):
             sage: k(3).sqrt()
             Traceback (most recent call last):
             ...
-            ValueError: must be a perfect square.
+            ValueError: must be a perfect square
 
         TESTS::
 
@@ -1066,9 +1081,9 @@ cdef class FiniteField_givaroElement(FinitePolyExtElement):
         elif cache.objectptr.characteristic() == 2:
             return make_FiniteField_givaroElement(cache, (cache.objectptr.cardinality() - 1 + self.element) / 2)
         elif extend:
-            raise NotImplementedError  # TODO: fix this once we have nested embeddings of finite fields
+            raise NotImplementedError  # TODO: use RingExtension or GF(p^(2*e))
         else:
-            raise ValueError("must be a perfect square.")
+            raise ValueError("must be a perfect square")
 
     cpdef _add_(self, right):
         """
@@ -1227,6 +1242,7 @@ cdef class FiniteField_givaroElement(FinitePolyExtElement):
 
         ALGORITHM:
 
+        Makes use of the internal representation of Givaro objects.
         Givaro objects are stored as integers `i` such that ``self`` `= a^i`,
         where `a` is a generator of `K` (though not necessarily the one
         returned by ``K.gens()``).  Now it is trivial to compute
@@ -1244,23 +1260,24 @@ cdef class FiniteField_givaroElement(FinitePolyExtElement):
             exp = _exp
 
         cdef Cache_givaro cache = self._cache
+        cdef GivaroGfq *objectptr = cache.objectptr
 
-        if (cache.objectptr).isOne(self.element):
+        if objectptr.isOne(self.element):
             return self
 
         elif exp == 0:
-            return make_FiniteField_givaroElement(cache, cache.objectptr.one)
+            return make_FiniteField_givaroElement(cache, objectptr.one)
 
-        elif (cache.objectptr).isZero(self.element):
+        elif objectptr.isZero(self.element):
             if exp < 0:
                 raise ZeroDivisionError('division by zero in finite field')
-            return make_FiniteField_givaroElement(cache, cache.objectptr.zero)
+            return make_FiniteField_givaroElement(cache, objectptr.zero)
 
-        cdef int order = (cache.order_c() - 1)
+        cdef int order = cache.order_c() - 1
         cdef int r = exp % order
 
         if r == 0:
-            return make_FiniteField_givaroElement(cache, cache.objectptr.one)
+            return make_FiniteField_givaroElement(cache, objectptr.one)
 
         cdef unsigned int r_unsigned
         if r < 0:
@@ -1271,12 +1288,12 @@ cdef class FiniteField_givaroElement(FinitePolyExtElement):
         cdef unsigned int order_unsigned = <unsigned int>order
         r = <int>(r_unsigned * elt_unsigned) % order_unsigned
         if r == 0:
-            return make_FiniteField_givaroElement(cache, cache.objectptr.one)
+            return make_FiniteField_givaroElement(cache, objectptr.one)
         return make_FiniteField_givaroElement(cache, r)
 
     cpdef _richcmp_(left, right, int op):
         """
-        Comparison of finite field elements is correct or equality
+        Comparison of finite field elements is correct for equality
         tests and somewhat random for ``<`` and ``>`` type of
         comparisons. This implementation performs these tests by
         comparing the underlying int representations.
@@ -1342,7 +1359,8 @@ cdef class FiniteField_givaroElement(FinitePolyExtElement):
 
         .. SEEALSO::
 
-            :meth:`sage.rings.finite_rings.element_base.FinitePolyExtElement.to_integer`
+            - :meth:`sage.rings.finite_rings.element_base.FinitePolyExtElement.to_integer`
+            - :meth:`_log_to_int`, identical to this method but returns a Sage :class:`~sage.rings.integer.Integer`.
 
         EXAMPLES::
 
@@ -1372,7 +1390,7 @@ cdef class FiniteField_givaroElement(FinitePolyExtElement):
             ...
             TypeError: not in prime subfield
         """
-        cdef int a = self._cache.log_to_int(self.element)
+        cdef unsigned int a = self._cache.log_to_int(self.element)
         if a < self._cache.objectptr.characteristic():
             return Integer(a)
         raise TypeError("not in prime subfield")
@@ -1386,6 +1404,10 @@ cdef class FiniteField_givaroElement(FinitePolyExtElement):
         `e = a_0 + a_1x + a_2x^2 + \cdots`, the int representation is
         `a_0 + a_1 p + a_2 p^2 + \cdots`.
 
+        .. SEEALSO::
+
+            - :meth:`_integer_representation`, identical to this method but returns a Python ``int``.
+
         EXAMPLES::
 
             sage: k.<b> = GF(5^2); k
@@ -1407,39 +1429,31 @@ cdef class FiniteField_givaroElement(FinitePolyExtElement):
         INPUT:
 
         - ``base`` -- non-zero field element
-        - ``order`` -- integer (optional), multiple of order of ``base``
+        - ``order`` -- integer (optional), multiple of order of ``base``.
+          This is only for backwards compatibility, it is not used in the
+          current implementation.
         - ``check`` -- boolean (default: ``False``): If set,
           test whether the given ``order`` is correct.
 
-        .. WARNING::
-
-            TODO -- This is currently implemented by solving the discrete
-            log problem -- which shouldn't be needed because of how finite field
-            elements are represented.
-
         EXAMPLES::
 
             sage: k.<b> = GF(5^2); k
             Finite Field in b of size 5^2
             sage: a = b^7
             sage: a.log(b)
             7
-
-        TESTS:
-
-        An example for ``check=True``::
-
-            sage: F.<t> = GF(3^5, impl='givaro')
-            sage: t.log(t, 3^4, check=True)
-            Traceback (most recent call last):
-            ...
-            ValueError: 81 is not a multiple of the order of the base
         """
-        b = self.parent()(base)
+        cdef FiniteField_givaroElement b = self.parent()(base)
+        if b.is_zero():
+            raise ValueError
         if (order is not None) and check and not (b**order).is_one():
             raise ValueError(f"{order} is not a multiple of the order of the base")
-
-        return sage.groups.generic.discrete_log(self, b, ord=order)
+        cdef int multiplicative_group_order = self._cache.order_c() - 1
+        cdef int gcd_b = ai.c_gcd_int(multiplicative_group_order, b.element)
+        if self.element % gcd_b != 0:
+            raise ValueError('no logarithm exists')
+        cdef int b_order = multiplicative_group_order / gcd_b
+        return Integer(self.element / gcd_b * <long long> ai.c_inverse_mod_int(b.element / gcd_b, b_order) % b_order)
 
     def _int_repr(FiniteField_givaroElement self):
         r"""
@@ -1570,25 +1584,9 @@ cdef class FiniteField_givaroElement(FinitePolyExtElement):
             sage: (b^6).multiplicative_order()
             4
         """
-        # TODO -- I'm sure this can be made vastly faster
-        # using how elements are represented as a power of the generator ??
-
-        if self._multiplicative_order is not None:
-            return self._multiplicative_order
-        else:
-            if self.is_zero():
-                raise ArithmeticError("Multiplicative order of 0 not defined.")
-            n = (self._cache).order_c() - 1
-            order = Integer(1)
-            for p, e in sage.arith.misc.factor(n):
-                # Determine the power of p that divides the order.
-                a = self**(n / (p**e))
-                while a != 1:
-                    order = order * p
-                    a = a**p
-
-            self._multiplicative_order = order
-            return order
+        if self.is_zero():
+            raise ArithmeticError("multiplicative order of 0 not defined")
+        return Integer((self._cache.order_c() - 1) / ai.c_gcd_int(self.element, self._cache.order_c() - 1))
 
     def __copy__(self):
         """

src/sage/tests/books/judson-abstract-algebra/finite-sage.py

@@ -102,6 +102,5 @@
     sage: (3*a^3 + 2*a^2 + a + 3).log(a^2 + 4*a + 4)
     Traceback (most recent call last):
     ...
-    ValueError: no discrete log of 3*a^3 + 2*a^2 + a + 3 found
-    to base a^2 + 4*a + 4
+    ValueError: no logarithm exists
 """