Bug in AbelianGroup.Subgroup
#36974
Labels
Comments
4 tasks
vbraun
pushed a commit
to vbraun/sage
that referenced
this issue
Jan 16, 2024
…`, 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
vbraun
pushed a commit
to vbraun/sage
that referenced
this issue
Jan 16, 2024
…`, 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
vbraun
pushed a commit
to vbraun/sage
that referenced
this issue
Jun 5, 2024
Reverted changes made in sagemath#36986 The bug in sagemath#36974 remains unsolved. Efforts are being made in sagemath#37744 to resolve the bug. Fixes sagemath#37744 ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [x] The title is concise and informative. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [x] I have updated the documentation and checked the documentation preview. ### ⌛ Dependencies URL: sagemath#37850 Reported by: Aman Moon Reviewer(s):
vbraun
pushed a commit
to vbraun/sage
that referenced
this issue
Jun 8, 2024
Reverted changes made in sagemath#36986 The bug in sagemath#36974 remains unsolved. Efforts are being made in sagemath#37744 to resolve the bug. Fixes sagemath#37744 ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [x] The title is concise and informative. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [x] I have updated the documentation and checked the documentation preview. ### ⌛ Dependencies URL: sagemath#37850 Reported by: Aman Moon Reviewer(s):
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Steps To Reproduce
Problem: The length of
G0.gens()andG0.gens_orders()should be the same.Additional Information
The
Subgroupis computed here. The issue seems to be that the GAP functionsAbelianInvariantsdoesn't actually return the orders of the generators inGeneratorsOfGroup, as demonstrated below:The obvious fix would be to replace
.AbelianInvariantson line 1710 with, well,[g.Order().sage() for g in H.GeneratorsOfGroup()]or something like that. If any GAP users have a better solution that returns the order of all (finite?) generators, please suggest it, since I haven't used GAP before.The text was updated successfully, but these errors were encountered: