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.

Sign up for GitHub

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

Jump to bottom

sage.groups.generic: Fix incorrect identity testing #37257

Merged
merged 7 commits into from
Mar 25, 2024
Merged

sage.groups.generic: Fix incorrect identity testing #37257

Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b9be7ab
sage.groups.generic: Fix incorrect identity test
grhkm21 Feb 8, 2024
d38462f
sage.groups.generic: Allow generic operation in `order_from_multiple`
grhkm21 Feb 8, 2024
bbb0649
Docs 🔁
grhkm21 Feb 8, 2024
0fae8c5
Update src/sage/groups/generic.py
grhkm21 Feb 11, 2024
cb1a79b
use `.zero()` instead of (0)
grhkm21 Feb 11, 2024
778cad8
apply review changes 📖
grhkm21 Feb 11, 2024
e9ce5c1
fix weird failing doctest...
grhkm21 Feb 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 33 additions & 20 deletions src/sage/groups/generic.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,6 @@

from sage.arith.misc import integer_ceil, integer_floor, xlcm
from sage.arith.srange import xsrange
from sage.misc.functional import log
from sage.misc.misc_c import prod
import sage.rings.integer_ring as integer_ring
import sage.rings.integer
Expand Down Expand Up @@ -171,9 +170,11 @@
sage: E = EllipticCurve('389a1')
sage: P = E(-1,1)
sage: multiple(P, 10, '+')
(645656132358737542773209599489/22817025904944891235367494656 : 525532176124281192881231818644174845702936831/3446581505217248068297884384990762467229696 : 1)
(645656132358737542773209599489/22817025904944891235367494656 :
525532176124281192881231818644174845702936831/3446581505217248068297884384990762467229696 : 1)
sage: multiple(P, -10, '+')
(645656132358737542773209599489/22817025904944891235367494656 : -528978757629498440949529703029165608170166527/3446581505217248068297884384990762467229696 : 1)
(645656132358737542773209599489/22817025904944891235367494656 :
-528978757629498440949529703029165608170166527/3446581505217248068297884384990762467229696 : 1)
"""
from operator import inv, mul, neg, add

Expand Down Expand Up @@ -332,11 +333,11 @@
P0 = P.parent()(0)
self.op = add
else:
self.op = op
if P0 is None:
raise ValueError("P0 must be supplied when operation is neither addition nor multiplication")
if op is None:
raise ValueError("op() must both be supplied when operation is neither addition nor multiplication")
self.op = op

Check warning on line 340 in src/sage/groups/generic.py

View check run for this annotation

Codecov / codecov/patch

src/sage/groups/generic.py#L340 

Added line #L340 was not covered by tests

self.P = copy(P)
self.Q = copy(P0)
Expand Down Expand Up @@ -458,6 +459,7 @@
inverse = inv
op = mul
elif operation in addition_names:
# Should this be replaced with .zero()? With an extra AttributeError handler?
identity = a.parent()(0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think .zero() should be faster, but indeed not all additive groups have a .zero() method...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took the violent approach of replacing it with .zero() and fixing the errors. I only found a few use cases of sage.groups.generic in the entire Sage, so I fixed those (only required fixing homset::zero)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks promising! Just double-checking: The change from .codomain() to .extended_codomain() in src/sage/schemes/generic/homset.py is an essentially unrelated bugfix, right? And this is what causes the point (a : a) in that doctest in src/sage/schemes/projective/projective_morphism.py to be normalized properly?

Copy link
Contributor Author

@grhkm21 grhkm21 Feb 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes they're two unrelated fixes (sorry!!), the extended_codomain is to imitate what schemes/projective/projective_homset.py does (and makes more sense). The normalisation is a separate problem.

It is caused by the following stripped code:

sage: F.<a> = GF(4)
....: ProjectiveSpace(ZZ, 1)._point(S(F), [a, a])
....: ProjectiveSpace(F, 1)._point(S(F), [a, a])
(a : a)
(1 : 1)

(This caller difference is introduced by the extended_codomain to codomain change.) This is because the two lines call different constructors

sage/src/sage/schemes/projective/projective_space.py

Lines 823 to 836 in 0c390a0

def _point(self, *args, **kwds):
"""
Construct a point.
For internal use only. See :mod:`morphism` for details.
TESTS::
sage: P2.<x,y,z> = ProjectiveSpace(2, GF(3))
sage: point_homset = P2._point_homset(Spec(GF(3)), P2)
sage: P2._point(point_homset, [1,2,3])
(2 : 1 : 0)
"""
return SchemeMorphism_point_projective_ring(*args, **kwds)
callls SchemeMorphism_point_projective_ring.

sage/src/sage/schemes/projective/projective_space.py

Lines 2271 to 2284 in 0c390a0

def _point(self, *args, **kwds):
"""
Construct a point.
For internal use only. See :mod:`morphism` for details.
TESTS::
sage: P2.<x,y,z> = ProjectiveSpace(2, GF(3))
sage: point_homset = P2._point_homset(Spec(GF(3)), P2)
sage: P2._point(point_homset, [1,2,3])
(2 : 1 : 0)
"""
return SchemeMorphism_point_projective_finite_field(*args, **kwds)
calls SchemeMorphism_point_projective_finite_field which allows division.

I think this is a separate problem, where ._point shouldn't care about the caller's type but rather the argument (homset)'s type, but again it's a separate problem. It's a separate problem.

inverse = neg
op = add
Expand All @@ -469,7 +471,7 @@
if lb < 0 or ub < lb:
raise ValueError("bsgs() requires 0<=lb<=ub")

if a.is_zero() and not b.is_zero():
if a == identity and b != identity:
raise ValueError("no solution in bsgs()")

ran = 1 + ub - lb # the length of the interval
Expand All @@ -479,7 +481,7 @@

if ran < 30: # use simple search for small ranges
d = c
# for i,d in multiples(a,ran,c,indexed=True,operation=operation):
# for i,d in multiples(a,ran,c,indexed=True,operation=operation):
grhkm21 marked this conversation as resolved.
Show resolved Hide resolved
for i0 in range(ran):
i = lb + i0
if identity == d: # identity == b^(-1)*a^i, so return i
Expand Down Expand Up @@ -1187,11 +1189,13 @@


def order_from_multiple(P, m, plist=None, factorization=None, check=True,
operation='+'):
operation='+', identity=None, inverse=None, op=None):
r"""
Generic function to find order of a group element given a multiple
of its order.

See :meth:`bsgs` for full explanation of the inputs.

INPUT:

- ``P`` -- a Sage object which is a group element;
Expand All @@ -1201,9 +1205,12 @@
really is a multiple of the order;
- ``factorization`` -- the factorization of ``m``, or ``None`` in which
case this function will need to factor ``m``;
- ``plist`` -- a list of the prime factors of ``m``, or ``None`` - kept for compatibility only,
- ``plist`` -- a list of the prime factors of ``m``, or ``None``. Kept for compatibility only,
prefer the use of ``factorization``;
- ``operation`` -- string: ``'+'`` (default) or ``'*'``.
- ``operation`` -- string: ``'+'`` (default), ``'*'`` or ``None``;
- ``identity`` -- the identity element of the group;
- ``inverse()`` -- function of 1 argument ``x``, returning inverse of ``x``;
- ``op()`` - function of 2 arguments ``x``, ``y`` returning ``x*y`` in the group.

.. note::

Expand Down Expand Up @@ -1257,14 +1264,23 @@
elif operation in addition_names:
identity = P.parent()(0)
else:
raise ValueError("unknown group operation")
if identity is None or inverse is None or op is None:
raise ValueError("identity, inverse and operation must all be specified")

Check warning on line 1268 in src/sage/groups/generic.py

View check run for this annotation

Codecov / codecov/patch

src/sage/groups/generic.py#L1267-L1268 

Added lines #L1267 - L1268 were not covered by tests

def _multiple(A, B):
return multiple(A,
B,
operation=operation,
identity=identity,
inverse=inverse,
op=op)

if P == identity:
return Z.one()

M = Z(m)
if check:
assert multiple(P, M, operation=operation) == identity
assert _multiple(P, M) == identity
grhkm21 marked this conversation as resolved.
Show resolved Hide resolved

if factorization:
F = factorization
Expand Down Expand Up @@ -1293,7 +1309,7 @@
p, e = L[0]
e0 = 0
while (Q != identity) and (e0 < e - 1):
Q = multiple(Q, p, operation=operation)
Q = _multiple(Q, p)
e0 += 1
if Q != identity:
e0 += 1
Expand All @@ -1312,12 +1328,8 @@
L2 = L[k:]
# recursive calls
o1 = _order_from_multiple_helper(
multiple(Q, prod([p**e for p, e in L2]), operation),
L1,
sum_left)
o2 = _order_from_multiple_helper(multiple(Q, o1, operation),
L2,
S - sum_left)
_multiple(Q, prod([p**e for p, e in L2])), L1, sum_left)
o2 = _order_from_multiple_helper(_multiple(Q, o1), L2, S - sum_left)
return o1 * o2

return _order_from_multiple_helper(P, F, sage.functions.log.log(float(M)))
Expand Down Expand Up @@ -1410,6 +1422,7 @@

return order_from_multiple(P, m, operation=operation, check=False)


def has_order(P, n, operation='+'):
r"""
Generic function to test if a group element `P` has order
Expand Down Expand Up @@ -1497,8 +1510,8 @@

fl = fn[::2]
fr = fn[1::2]
l = prod(p**k for p,k in fl)
r = prod(p**k for p,k in fr)
l = prod(p**k for p, k in fl)
r = prod(p**k for p, k in fr)
L, R = mult(Q, r), mult(Q, l)
return _rec(L, fl) and _rec(R, fr)

Expand Down
Loading