Fixed a bug in AbelianGroup.Subgroup.gens_orders(), which returns the reduced order of the subgroup
#36986
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
amanmoon
force-pushed
the
bug/AbelianGroup.Subgroup
branch
from
25541c0
to
ee16f2c
Compare
amanmoon
force-pushed
the
bug/AbelianGroup.Subgroup
branch
from
ee16f2c
to
2605463
Compare
amanmoon
changed the title
bug in gens_orders() that initially used to return the reduced order of the subgroup.bug in AbelianGroup.Subgroup.gens_orders() that initially used to return the reduced order of the subgroup.
tscrim
requested changes
Jan 5, 2024
There was a problem hiding this comment.
I think the general idea of not passing invs as the generators of the subgroup is the correct idea. However, you are effectively creating redundant information with the additional argument. Plus there becomes an inconsistency with the variable names being created. You can see this by
sage: S.inject_variables()
Defining f0, f1
sage: f1
---------------------------------------------------------------------------
NameError Traceback (most recent call last)
Cell In[13], line 1
----> 1 f1
NameError: name 'f1' is not defined
Instead, you should simply pass the gens_orders up as the first argument to AbelianGroup_class.__init__ (after self of course).
…nd reverted few changes
tscrim
reviewed
Jan 10, 2024
tscrim
changed the title
bug in AbelianGroup.Subgroup.gens_orders() that initially used to return the reduced order of the subgroup.AbelianGroup.Subgroup.gens_orders(), which returns the reduced order of the subgroup
|
merge conflict |
|
@vbraun, I have made the necessary changes. |
|
@amanmoon That's not what the automated checker is saying. Please try again by either merging or rebasing (I recommend the latter) over the most recent develop branch. |
|
Documentation preview for this PR (built with commit 300c530; changes) is ready! 🎉 |
|
Thanks. This is back to a positive review. |
vbraun
pushed a commit
to vbraun/sage
that referenced
this pull request
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 pull request
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
|
This has broken |
amanmoon
added a commit
to amanmoon/sage
that referenced
this pull request
Apr 22, 2024
4 tasks
vbraun
pushed a commit
to vbraun/sage
that referenced
this pull request
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 pull request
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The
gens_orders()method will returns the correct order after these changes.Created a new variable
gens_orders:Modified the
AbelianGroup_classclass to include an additional argument, from which gens_orders is passed and assigned toself.gens_orders:Also, tests have been written for the
gens_ordersmethod:Additionally, performed code cleanup.
Fixes #36974
Changing the
invsvariable directly to[order_sage for g in H.GeneratorsOfGroup() if (order_sage := g.Order().sage()) is not infinity]here causes series of errors, as other variables depend on it.📝 Checklist