Add new function group which constructs groups from various inputs
#5686
Conversation
fingolfin
changed the title
group which constructs groups from various inputs
fingolfin
added
enhancement
ThomasBreuer
left a comment
ThomasBreuer
left a comment
There was a problem hiding this comment.
Eventually, the new meaning of group should be documented, with a remark that for "higher level objects" such as cosets, transversals, or invariant rings, calling group means to fetch a stored group.
test/Groups/constructors.jl
Outdated
| @test degree(G2) == 4 | ||
|
|
||
| @test order(G1) == order(G2) | ||
| @test is_isomorphic(G1, G2) |
There was a problem hiding this comment.
| @test is_isomorphic(G1, G2) | |
| @test G1 == G2 |
We want that the two permutation_group results have the same elements, not just that the two groups are abstractly isomorphic.
test/Groups/constructors.jl
Outdated
|
|
||
| @test order(H1) == order(G1) | ||
| @test is_isomorphic(H1, G1) | ||
| @test is_isomorphic(H2, G2) |
src/Groups/group_constructors.jl
Outdated
| isempty(gs) && throw(ArgumentError("need at least one generator")) | ||
| n = maximum(degree, gs) | ||
| return permutation_group(n, PermGroupElem[gs...]) | ||
| end |
There was a problem hiding this comment.
Wouldn't the two new permutation_group methods better be added to perm.jl, where the method for the case of a prescribed degree is?
(And the docstring for that method could be extended in order to cover the case that no degree is given.)
There was a problem hiding this comment.
okay got it, thankyou for the suggestion.
Thankyou for your response, I agree. once the semantics of group are clearer, this would be good to document. |
|
also, the original issue mentioned extending group(...) to other generator types |
|
@varuntrehan7 Once you think that you have reacted to all comments, please mark this PR as "ready to review". This not only applies to this PR, but also to most of your other ones |
i will do that, thanks! |
|
i will now work to add matrix groups before we merge permutation groups so if there are any complications we can clear them up together, thus converted this back to a draft. Thanks! |
varuntrehan7
force-pushed
the
vt/group-constructor
branch
from
bd1ea0e to
67569d1
Compare
|
i added matrix groups @ThomasBreuer, please have a look. Thanks! |
ThomasBreuer
left a comment
ThomasBreuer
left a comment
There was a problem hiding this comment.
perhaps one small change in the delegation
src/Groups/group_constructors.jl
Outdated
| group(gs::AbstractVector{<:PermGroupElem}) = permutation_group(gs) | ||
|
|
||
| # Matrix group from matrix generators | ||
| group(g::MatElem, gs::MatElem...) = group(MatElem[g, gs...]) |
There was a problem hiding this comment.
| group(g::MatElem, gs::MatElem...) = group(MatElem[g, gs...]) | |
| group(g::MatElem, gs::MatElem...) = matrix_group(MatElem[g, gs...]) |
|
On Wed, Jan 14, 2026 at 08:33:30AM -0800, Thomas Breuer wrote:
@ThomasBreuer approved this pull request.
perhaps one small change in the delegation
> @@ -102,6 +102,14 @@ and `false` otherwise.
"""
@gapattribute is_isomorphic_to_alternating_group(G::GAPGroup) = GAP.Globals.IsAlternatingGroup(GapObj(G))::Bool
+# Generic `group` convenience: for permutations that agrees with `permutation_group`
+group(g::PermGroupElem, gs::PermGroupElem...) = permutation_group(g, gs...)
+group(gs::AbstractVector{<:PermGroupElem}) = permutation_group(gs)
+
+# Matrix group from matrix generators
+group(g::MatElem, gs::MatElem...) = group(MatElem[g, gs...])
```suggestion
group(g::MatElem, gs::MatElem...) = matrix_group(MatElem[g, gs...])
```
What about also adding `check` to pass it through to the matrix_group
function?
… --
Reply to this email directly or view it on GitHub:
#5686 (review)
You are receiving this because you are subscribed to this thread.
Message ID: ***@***.***>
|
good point, thanks. I have added the check. |
5 participants
The original issue also mentions extending group(...) to other generator types
(e.g. matrix generators or GAP groups). So far i have only added
permutation generators to keep the change small and reviewable.
i will add them next, thanks!