Skip to content

Commit

Permalink
sagemathgh-36986: Fixed a bug in `AbelianGroup.Subgroup.gens_orders()…
Browse files Browse the repository at this point in the history
…`, which returns the reduced order of the subgroup

    
The `gens_orders()` method will returns the correct order after these
changes.
<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
Created a new variable `gens_orders`:
```python
gens_orders = [order_sage for g in H.GeneratorsOfGroup() if (order_sage
:= g.Order().sage()) is not infinity]
```
Modified the `AbelianGroup_class` class to include an additional
argument, from which gens_orders is passed and assigned to
`self.gens_orders`:
```python
def __init__(self, generator_orders, names, gens_orders=None,
category=None):
self._gens_orders = generator_orders if gens_orders is None else
gens_orders
```
Also, tests have been written for the `gens_orders` method:
```sage
sage: G, (g0, g1) = AbelianGroup(2, [48, 0]).objgens()
sage: G0 = G.subgroup([g0])
sage: len(G0.gens()) == len(G0.gens_orders())
True
```
Additionally, performed code cleanup.
<!-- Why is this change required? What problem does it solve? -->
Fixes sagemath#36974

Changing the `invs` variable directly to `[order_sage for g in
H.GeneratorsOfGroup() if (order_sage := g.Order().sage()) is not
infinity]` [here](https://github.com/sagemath/sage/blob/e249befd47ad8610
f74372533a2943ff7b2dde94/src/sage/groups/abelian_gps/abelian_group.py#L1
710) causes series of errors, as other variables depend on it.
### 📝 Checklist


<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->

- [x] The title is concise, informative, and self-explanatory.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [x] I have created tests covering the changes.

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#36986
Reported by: Aman Moon
Reviewer(s): Travis Scrimshaw
  • Loading branch information
Release Manager committed Jan 16, 2024
2 parents e968736 + 300c530 commit 650332d
Showing 1 changed file with 18 additions and 8 deletions.
26 changes: 18 additions & 8 deletions src/sage/groups/abelian_gps/abelian_group.py
Original file line number Diff line number Diff line change
Expand Up @@ -380,6 +380,7 @@ def _normalize(n, gens_orders=None, names="f"):
names = tuple(names)
return (gens_orders, names)


def AbelianGroup(n, gens_orders=None, names="f"):
r"""
Create the multiplicative abelian group in `n` generators
Expand Down Expand Up @@ -444,6 +445,7 @@ def AbelianGroup(n, gens_orders=None, names="f"):
M = AbelianGroup_class(gens_orders, names)
return M


def is_AbelianGroup(x):
"""
Return True if ``x`` is an Abelian group.
Expand Down Expand Up @@ -535,7 +537,7 @@ def __init__(self, generator_orders, names, category=None):
"""
assert isinstance(names, (str, tuple))
assert isinstance(generator_orders, tuple)
assert all(isinstance(order,Integer) for order in generator_orders)
assert all(isinstance(order, Integer) for order in generator_orders)
self._gens_orders = generator_orders
n = len(generator_orders)
names = normalize_names(n, names)
Expand Down Expand Up @@ -943,7 +945,7 @@ def gens(self):
sage: [g.order() for g in F.gens()]
[+Infinity, +Infinity, +Infinity, 3, 2]
"""
return tuple( self.gen(i) for i in range(self.ngens()) )
return tuple(self.gen(i) for i in range(self.ngens()))

def gens_orders(self):
"""
Expand Down Expand Up @@ -978,6 +980,10 @@ def gens_orders(self):
TESTS::
sage: G, (g0, g1) = AbelianGroup(2, [48, 0]).objgens()
sage: G0 = G.subgroup([g0])
sage: len(G0.gens()) == len(G0.gens_orders())
True
sage: F = AbelianGroup(3, [2], names='abc')
sage: list(map(type, F.gens_orders()))
[<class 'sage.rings.integer.Integer'>,
Expand Down Expand Up @@ -1175,7 +1181,7 @@ def random_element(self):
order = g.order()
if order is infinity:
order = 42 # infinite order; randomly chosen maximum
result *= g**(randint(0, order))
result *= g ** (randint(0, order))
return result

def _repr_(self) -> str:
Expand Down Expand Up @@ -1554,8 +1560,8 @@ def subgroup_reduced(self, elts, verbose=False):
rel_lattice = X.span([X.gen(i) * self.gens_orders()[i] for i in range(d)])
isect = elt_lattice.intersection(rel_lattice)
mat = matrix([elt_lattice.coordinate_vector(x) for x in isect.gens()]).change_ring(ZZ)
D,U,V = mat.smith_form()
new_basis = [(elt_lattice.linear_combination_of_basis((~V).row(i)).list(), D[i,i]) for i in range(U.ncols())]
D, U, V = mat.smith_form()
new_basis = [(elt_lattice.linear_combination_of_basis((~V).row(i)).list(), D[i, i]) for i in range(U.ncols())]
return self.subgroup([self([x[0][i] % self.gens_orders()[i]
for i in range(d)]) for x in new_basis if x[1] != 1])

Expand Down Expand Up @@ -1658,7 +1664,7 @@ def __init__(self, ambient, gens, names="f", category=None):
sage: B = A.subgroup([a^3, b, c, d, e^2]); B
Multiplicative Abelian subgroup isomorphic to C4 x C5 x C5 x C7 generated by {b, c, d, e^2}
sage: B.gens_orders()
(4, 5, 5, 7)
(5, 5, 7, 4)
sage: A = AbelianGroup(4,[1009, 2003, 3001, 4001], names="abcd")
sage: a,b,c,d = A.gens()
sage: B = A.subgroup([a^3,b,c,d])
Expand All @@ -1683,7 +1689,7 @@ def __init__(self, ambient, gens, names="f", category=None):
Multiplicative Abelian subgroup isomorphic to C2 x C3 x Z
generated by {a, b^2, c}
sage: F.gens_orders()
(2, 3, 0)
(3, 2, 0)
sage: F.gens()
(a, b^2, c)
sage: F.order()
Expand All @@ -1709,16 +1715,20 @@ def __init__(self, ambient, gens, names="f", category=None):
H = libgap(ambient).Subgroup(H_gens)

invs = H.TorsionSubgroup().AbelianInvariants().sage()
gens_orders = tuple([ZZ(order_sage) for g in H.GeneratorsOfGroup()
if (order_sage := g.Order().sage()) is not infinity])

rank = len([1 for g in H.GeneratorsOfGroup()
if g.Order().sage() is infinity])
invs += [0] * rank

gens_orders += (ZZ.zero(),) * rank
self._abinvs = invs
invs = tuple(ZZ(i) for i in invs)

if category is None:
category = Groups().Commutative().Subobjects()
AbelianGroup_class.__init__(self, invs, names, category=category)
AbelianGroup_class.__init__(self, gens_orders, names, category=category)

def __contains__(self, x):
"""
Expand Down

0 comments on commit 650332d

Please sign in to comment.