-
Notifications
You must be signed in to change notification settings - Fork 120
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.
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
support group extensions of matrix groups #4013
Conversation
- admit `Oscar.GAPGroupElem` not only `Oscar.BasicGAPGroupElem` in evaluating 2-cochains (we need in particular `Oscar.MatrixGroupElem`) - add a `isomorphism(::Type{FPGroup}, A::AbstractAlgebra.Generic.FreeModule)` method, which is needed by `extension` if we want to support G-modules created from matrix groups acting on vector spaces - add tests
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4013 +/- ##
==========================================
+ Coverage 84.59% 84.68% +0.09%
==========================================
Files 596 600 +4
Lines 81971 82640 +669
==========================================
+ Hits 69341 69987 +646
- Misses 12630 12653 +23
|
Mostly happy, but in Cohomology.m we already have
I think we need to consolidate this (and make a consistent interface?) At least as a promise, the "old" function is more general as in accepts not only prime fields, and not only free_modules. |
What about also adding (maybe later)
|
Concerning a consistent interface, the code anyhow needs Concerning |
The existing functions will have to be renamed into |
O.k., I can adjust the pull request according to the above discussion. |
- rename `natural_gmodule` to `regular_gmodule`, extend its documentation, export it - introduce `natural_module`, export it - add tests for `natural_gmodule` and `regular_gmodule`
Concerning the idea
There is already a matching |
and support `is_elementary_abelian` for `FinGenAbGroup`
- generalize `isomorphism(FPGroup, M::...; on_gens=true)` and `isomorphism(PcGroup, M::...: on_gens=true)` to `M::AbstractAlgebra.Generic.FreeModule{FqFieldElem}` as in the functions `fp_group_with_isomorphism`, `pc_group_with_isomorphism` in `experimental/GModule`, delegate from the latter to the former - remove the variant `fp_group_with_isomorphism(g::Vector{<:Oscar.GAPGroupElem})`, since it did in fact not depend on `g` but on `parent(g[1])`. - move the general `relators(G::GAPGroup)` methods to `src/Groups`, changed `relations` to delegate to `relators` - change `free_group(G::FPGroup)` to return `G` if `G` is free - move `underlying_word(g::FPGroupElem)` to `src/Groups`, changed it to return `g` if its parent is free - add `isomorphism(T, G::T)` methods that return an identity map - improve the `isomorphism(::Type{FPGroup}, A::FinGenAbGroup)` in the sense that only commutator relators for one half of the matrix are needed - add some tests - removed some tests: Due to the fact that `relators` is now defined for more types of groups, one may get fewer errors or different errors than before (and it may take longer until one gets these errors)
on first glance: looks good! |
I'll discuss this next week with @fieker and @ThomasBreuer to see if we can merge (and I'll try to get in a review I before that, too) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me as well, @fieker should be in the office within the next hour and we can then let him merge it
* support group extensions of matrix groups - admit `Oscar.GAPGroupElem` not only `Oscar.BasicGAPGroupElem` in evaluating 2-cochains (we need in particular `Oscar.MatrixGroupElem`) - add a `isomorphism(::Type{FPGroup}, A::AbstractAlgebra.Generic.FreeModule)` method, which is needed by `extension` if we want to support G-modules created from matrix groups acting on vector spaces - add tests * add a test in order to increase coverage * `natural_gmodule` and `regular_gmodule` - rename `natural_gmodule` to `regular_gmodule`, extend its documentation, export it - introduce `natural_module`, export it - add tests for `natural_gmodule` and `regular_gmodule` * add `elementary_abelian_group` and support `is_elementary_abelian` for `FinGenAbGroup` * next iteration, motivated by the comments - generalize `isomorphism(FPGroup, M::...; on_gens=true)` and `isomorphism(PcGroup, M::...: on_gens=true)` to `M::AbstractAlgebra.Generic.FreeModule{FqFieldElem}` as in the functions `fp_group_with_isomorphism`, `pc_group_with_isomorphism` in `experimental/GModule`, delegate from the latter to the former - remove the variant `fp_group_with_isomorphism(g::Vector{<:Oscar.GAPGroupElem})`, since it did in fact not depend on `g` but on `parent(g[1])`. - move the general `relators(G::GAPGroup)` methods to `src/Groups`, changed `relations` to delegate to `relators` - change `free_group(G::FPGroup)` to return `G` if `G` is free - move `underlying_word(g::FPGroupElem)` to `src/Groups`, changed it to return `g` if its parent is free - add `isomorphism(T, G::T)` methods that return an identity map - improve the `isomorphism(::Type{FPGroup}, A::FinGenAbGroup)` in the sense that only commutator relators for one half of the matrix are needed - add some tests - removed some tests: Due to the fact that `relators` is now defined for more types of groups, one may get fewer errors or different errors than before (and it may take longer until one gets these errors) * fix an `elementary_abelian_group` method
admit
Oscar.GAPGroupElem
not onlyOscar.BasicGAPGroupElem
in evaluating 2-cochains (we need in particularOscar.MatrixGroupElem
)add a
isomorphism(::Type{FPGroup}, A::AbstractAlgebra.Generic.FreeModule)
method, which is needed byextension
if we want to support G-modules created from matrix groups acting on vector spacesadd tests