introduce `SubPcGroup` #3166

ThomasBreuer · 2024-01-10T12:26:20Z

This is a first attempt to implement what was sketched in #2672.
I am not sure that we really want this:

Admitting different types for a group and a subgroup was apparently not allowed up to now, several signatures of functions have to be "relaxed" in order to support different types.
Functions returning arrays of subgroups (typically series such as derived_series) will now create the full group (if it occurs) with another type; one has to be aware of different types for the same mathematical object.

If we want to proceed like this then the next step is as follows.

Fix functions like intersection which expect arguments of the same type up to now.
Adjust the doctests.
Adjust/extend the documentation.
Add tests.
Do the same for FpGroup/SubFpGroup.

src/Groups/GAPGroups.jl

fingolfin · 2024-01-22T11:54:02Z

This is a first attempt to implement what was sketched in #2672. I am not sure that we really want this:

Admitting different types for a group and a subgroup was apparently not allowed up to now, several signatures of functions have to be "relaxed" in order to support different types.

That seems fine to me.

Functions returning arrays of subgroups (typically series such as derived_series) will now create the full group (if it occurs) with another type; one has to be aware of different types for the same mathematical object.

I also think this is fine, and in fact just reflects the reality in GAP, which tries to paper of it but doesn't really succeed. So I actually think it is better that we make this explicit. This way at least the user can immediately tell if a given group is the "full" group or not, and we have a chance of explaining the situation in the docstring for SubPcGroup.

If we want to proceed like this then the next step is as follows.

Fix functions like intersection which expect arguments of the same type up to now.

Adjust the doctests.

Adjust/extend the documentation.

Add tests.

Do the same for FpGroup/SubFpGroup.

src/Groups/GAPGroups.jl

fingolfin · 2024-01-22T12:10:53Z

src/Groups/group_constructors.jl

@@ -110,11 +117,12 @@ function cyclic_group(::Type{T}, n::Union{IntegerUnion,PosInf}) where T <: GAPGr
  return T(GAP.Globals.CyclicGroup(_gap_filter(T), GAP.Obj(n))::GapObj)
 end

-function cyclic_group(::Type{PcGroup}, n::Union{IntegerUnion,PosInf})
+# special handling for pc groups: distinguish on the GAP side
+function cyclic_group(::Type{T}, n::Union{IntegerUnion,PosInf}) where T <: Union{PcGroup, SubPcGroup}


This creates a new PcGroup, which would be the "whole" group, so should we really allow SubPcGroup here?

@ThomasBreuer did you see my question from January there? I don't terribly mind having this (it may make some generic code easier to write), but I wonder: does cyclic_group(SubPcGroup, 5) really work correctly, and return something of type SubPcGroup? We don't have a test for that, do we?

Yes, cyclic_group(SubPcGroup, 5) works.
(I had intended to add a test for this, but somehow this got lost.)

fingolfin · 2024-01-22T12:12:13Z

src/Groups/homomorphisms.jl

@@ -517,6 +524,29 @@ function isomorphism(::Type{T}, G::GAPGroup) where T <: Union{FPGroup, PcGroup,
   end::GAPGroupHomomorphism{typeof(G), T}
 end

+# If `G` is not a full pc group then switch to a full pc group in Oscar.


This, by the way, is something that I needed a lot in the past, and was always annoyingly complicated in GAP. So this is a nice bonus I did not even consider (at least I don't recall considering it :-) )

I think GAP is just missing something like IsomorphismFullPcGroup.

src/Groups/homomorphisms.jl

fingolfin · 2024-01-22T12:14:13Z

src/Groups/homomorphisms.jl

@@ -741,12 +771,14 @@ end
    GrpAbFinGen(G::T) where T <: GAPGroup
    PcGroup(G::T) where T <: Union{GAPGroup, GrpAbFinGen}
    pc_group(G::T) where T <: Union{GAPGroup, GrpAbFinGen}
+    SubPcGroup(G::T) where T <: Union{GAPGroup, GrpAbFinGen}
+    sub_pc_group(G::T) where T <: Union{GAPGroup, GrpAbFinGen}


Do we really want/need this? Maybe SubPcGroup on a PcGroup could be useful, in case one needs the "full group as a subgroup", but other than that?

src/Groups/types.jl

src/Groups/sub.jl

fingolfin · 2024-01-31T17:13:14Z

Just to document it somewhere in writing: We discussed this some time ago and the verdict was that yes, we want to proceed in this direction; while there are downsides, many of them actually already exists, but just are hidden; and there are also upsides. So...

fingolfin · 2024-02-28T09:55:20Z

@ThomasBreuer I hope this is still on your radar? :-)

ThomasBreuer · 2024-05-02T13:59:52Z

The failure of one of the book examples shows a conceptual problem. (Likely this example can be turned back to the old behaviour by an improvement on the GAP side, but this does not solve the general problem.)

The function quo for two groups states that the type of the return value depends on the situation: Anyhow a new group must be created, and GAP decides about a suitable representation; if the result is known to be a finite solvable group then a pc group is returned, otherwise a permutation group or a fp group are possible.

The GAP function NaturalHomomorphismByNormalSubgroup is allowed to return a mapping that is not (known to be) surjective. If this happens and if the Range of the mapping is a pc group then the quotient on the Oscar side can be only a SubPcGroup. (At some extra cost, we can check the surjectivity, and create a PcGroup in Oscar if yes.)

It looks as if we have to do something more specific on the GAP side if we want the results of quo to be either PcGroup or FPGroup or PermGroup.

(@fieker @fingolfin ?)

ThomasBreuer · 2024-05-03T11:30:13Z

The failure in test (1.10, book, ubuntu-latest) is due to an intended change of the output shown: The group in question has now a different type.

benlorenz · 2024-05-03T11:38:22Z

The failure in test (1.10, book, ubuntu-latest) is due to an intended change of the output shown: The group in question has now a different type.

Then you can adjust the output in (test/book/cornerstones/groups/actions.jlcon) to the new printing. Only the input must stay identical for 1.1 (i.e. current master).

codecov · 2024-05-04T11:02:59Z

Codecov Report

Attention: Patch coverage is 88.18182% with 26 lines in your changes are missing coverage. Please review.

Project coverage is 81.35%. Comparing base (357c46b) to head (c861c62).
Report is 18 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3166      +/-   ##
==========================================
+ Coverage   81.34%   81.35%   +0.01%     
==========================================
  Files         577      577              
  Lines       78618    78761     +143     
==========================================
+ Hits        63952    64077     +125     
- Misses      14666    14684      +18

Files	Coverage Δ
experimental/GModule/Brueckner.jl	`43.58% <100.00%> (ø)`
...ntal/SymmetricIntersections/src/representations.jl	`96.65% <100.00%> (ø)`
src/GAP/wrappers.jl	`96.73% <100.00%> (+0.02%)`	⬆️
src/Groups/GAPGroups.jl	`93.22% <100.00%> (+0.02%)`	⬆️
src/Groups/abelian_aut.jl	`97.57% <100.00%> (ø)`
src/Groups/action.jl	`97.16% <100.00%> (+0.02%)`	⬆️
src/Groups/cosets.jl	`86.87% <100.00%> (+0.16%)`	⬆️
src/Groups/directproducts.jl	`90.90% <ø> (ø)`
src/Groups/group_characters.jl	`95.49% <100.00%> (ø)`
src/Groups/libraries/atlasgroups.jl	`90.21% <100.00%> (-0.31%)`	⬇️
... and 7 more

... and 79 files with indirect coverage changes

fingolfin

Some remarks. Most are optional (better to get this merged sooner and then polish afterwards), but there are one or two were I wonder if the code is correct as it is...?

fingolfin · 2024-05-13T11:32:53Z

src/Groups/group_constructors.jl

@@ -110,11 +117,12 @@ function cyclic_group(::Type{T}, n::Union{IntegerUnion,PosInf}) where T <: GAPGr
  return T(GAP.Globals.CyclicGroup(_gap_filter(T), GAP.Obj(n))::GapObj)
 end

-function cyclic_group(::Type{PcGroup}, n::Union{IntegerUnion,PosInf})
+# special handling for pc groups: distinguish on the GAP side
+function cyclic_group(::Type{T}, n::Union{IntegerUnion,PosInf}) where T <: Union{PcGroup, SubPcGroup}


@ThomasBreuer did you see my question from January there? I don't terribly mind having this (it may make some generic code easier to write), but I wonder: does cyclic_group(SubPcGroup, 5) really work correctly, and return something of type SubPcGroup? We don't have a test for that, do we?

src/Groups/homomorphisms.jl

fingolfin · 2024-05-13T12:25:06Z

src/Groups/homomorphisms.jl

+# # Remove once Hecke lower bound is >= 0.14.1
+# @attributes MultTableGroup



Suggested change

# # Remove once Hecke lower bound is >= 0.14.1

# @attributes MultTableGroup

src/Groups/types.jl

test/Groups/homomorphisms.jl

test/Groups/libraries.jl

test/Groups/quotients.jl

fingolfin · 2024-05-14T23:14:21Z

Unfortunately this has conflicts again now :-(

It would be great if we could merge this tomorrow (before or after triage, whenever you have time to resolve it) so we can move on :-)

- add `_check_compatible` - remove requirements that several groups as arguments must have the same type; instead call `_check_compatible` inside

- add new GAP property `GroupGeneratorsDefinePresentation`, in order to check/cache whether a GAP group is the full group given by the underlying presentation, *and* whether the stored group generators correspond to the generators of this presentation - export `SubPcGroup`, `SubPcGroupElem` - relax type relations in various signatures, since now subgroups of a group can have a type different from that of the full group - change the parametrization of various types because now several types of groups may be involved: `GroupCoset` involves a group, a subgroup, and a group element. `GroupDoubleCoset` involves a group, two subgroups, and a group element. `SubgroupTransversal` involves a group and a subgroup - add a dictionary `_sub_types` that maps a group type to the type that is used for its (not necessariy proper) subgroups, adjust `_oscar_group` to create a group of the subgroup type (this is what `_oscar_group` is thought for), adjust `_get_type`, - support `on_gens = true` also for `isomorphism(PcGroup, G)` where possible - change some (types of) return values: - the default return type of `_isomorphic_gap_group(A::FinGenAbGroup)` from `PcGroup` to `SubPcGroup`, - the return value of `isomorphism_to_GAP_group(G::FinGenAbGroup)` to use a `SubPcGroup` not a `PcGroup`, - throw an error if `abelian_group(PcGroup, G)` is called for a group `G` whose generators do not fit to a supported pc presentation, - and throw an error if `isomorphism(PcGroup, A)` is called for an abelian group such that the generators of `A` do not correspond to the generators of the underlying presentation. Yes, this is not a good idea. The problem is that currently we cannot guarantee that the `gens` value of a newly constructed `PcGroup` isomorphic to a given abelian group corresponds to the generators in the defining presentation. (GAP's `IsPcGroup` groups support only prime relative orders, and we cannot use GAP's `IsPcpGroup` throughout because of bugs that show up in Oscar's CI tests.) (Question: Should the constructor `PcGroup(G::GapObj)`, where `G` is a non-full pc group in GAP, be allowed to replace `G` be a newly created independent group, or should it throw an error, and `pc_group` should do the switch to a new group before calling `PcGroup`?) - disable (temporarily) some `@inferred` calls in tests, due to situations where types of results depend on `_sub_types` - call `abelian_group(SubPcGroup, ...)` instead of `abelian_group(PcGroup, ...)` in several tests, where the latter is currently not supported (hopefully temporarily); analogously for `isomorphism(PcGroup, ...)` vs. `isomorphism(SubPcGroup, ...)` - disable (temporarily) two equality tests concerning mappings from/to `PcGroup`s or `SubPcGroup`s: Would it be o.k. to just regard the groups (the maps) as equal, independent of their types?

due to changed `sub_type` handling

- adjust doctest output to `SubPcGroup` objects - changed signature of `complements`, `complement_classes`, added tests - document `SubPcGroupElem`

`PcGroup` takes a GAP group and wraps it; it is not allowed to replace the group by a different GAP group, thus it must throw an exception if the GAP group is not suitable. (This is crucial for example in the creation of group homomorphisms, where the GAP objects stored in domain and codomain must fit to the `Source` and `Range` objects of the underlying GAP mapping.) `pc_group` may return an Oscar group that stores a different GAP group than the given one.

The function must be able to create a map to a `PcGroup`, also in the case that `abelian_group(PcGroup, exponents)` does not work, due to the requirements that the `gens` value of the result corresponds to the given element orders *and* is the defining polycyclic sequence of the result group.

This fixes a test. I am not sure whether we want a more strict use of embeddings, or to allow for a more sloppy use of group elements. (In the latter case, we will run into problems in examples involving `FinGenAbGroup`s.)

- fix a signature in a `.md` file - remove a type assertion that is no longer valid (a subgroup can have a different type)

I am not sure whether this is a good idea. On the one hand, users would not understand that a newly created quotient is represented as a `SubPcGroup`. (The starting point was changed output in an example for the Oscar book.) On the other hand, we have to work around a GAP feature that offers natural homomorphisms instead of natural epimorphisms (with the possibility to create maps that need not be surjective). Since we have to compute the image of the GAP mapping anyhow, the current change does not add expensive computations.

- add comments - add tests - improve error messages

ThomasBreuer · 2024-05-15T08:26:03Z

Unfortunately this has conflicts again now :-(

I find it rather surprising that there were not more conflicts.

fingolfin · 2024-05-15T08:44:53Z

@ThomasBreuer there is one unresolved review comment of mine from quite some ago where I ask about sub_pc_group -- can you please reply to that?

ThomasBreuer requested a review from fingolfin January 10, 2024 12:26

ThomasBreuer marked this pull request as draft January 10, 2024 12:26

lgoettgens reviewed Jan 10, 2024

View reviewed changes

src/Groups/GAPGroups.jl Outdated Show resolved Hide resolved

fingolfin reviewed Jan 22, 2024

View reviewed changes

src/Groups/types.jl Show resolved Hide resolved

fingolfin reviewed Jan 31, 2024

View reviewed changes

src/Groups/sub.jl Outdated Show resolved Hide resolved

ThomasBreuer mentioned this pull request Apr 19, 2024

wrong group order of a preimage of a group homomorphism gap-system/gap#5704

Closed

ThomasBreuer force-pushed the TB_SubPcGroup branch from 877bc2a to 6d0af6b Compare April 26, 2024 12:27

ThomasBreuer force-pushed the TB_SubPcGroup branch from 7fc9496 to 741533a Compare May 3, 2024 09:42

ThomasBreuer marked this pull request as ready for review May 3, 2024 11:34

lgoettgens closed this May 4, 2024

lgoettgens reopened this May 4, 2024

lgoettgens added the topic: groups label May 4, 2024

lgoettgens requested a review from fingolfin May 4, 2024 10:10

fingolfin reviewed May 13, 2024

View reviewed changes

ThomasBreuer force-pushed the TB_SubPcGroup branch from 350cba7 to c861c62 Compare May 14, 2024 21:50

ThomasBreuer added 6 commits May 15, 2024 09:10

introduce SubPcGroup

a262a05

address comments

4ca685e

next step towards SubPcGroup

9546868

- add `_check_compatible` - remove requirements that several groups as arguments must have the same type; instead call `_check_compatible` inside

@inferred tests work again

c5c0c1c

due to changed `sub_type` handling

adjust docstrings to signature changes

6950506

ThomasBreuer added 12 commits May 15, 2024 09:14

adjust serialization to type SubPcGroup

5ecdbcc

more adjustments

bc649aa

- adjust doctest output to `SubPcGroup` objects - changed signature of `complements`, `complement_classes`, added tests - document `SubPcGroupElem`

use an embedding

bcb1675

This fixes a test. I am not sure whether we want a more strict use of embeddings, or to allow for a more sloppy use of group elements. (In the latter case, we will run into problems in examples involving `FinGenAbGroup`s.)

adjust two doctests

7dd9569

more fixes

6572137

- fix a signature in a `.md` file - remove a type assertion that is no longer valid (a subgroup can have a different type)

fix cross-references

f7304a4

get rid of _get_type

e55c760

adjust output in an Oscar Book example

85e7ce9

address reviewer's remarks

0c07ce0

- add comments - add tests - improve error messages

ThomasBreuer force-pushed the TB_SubPcGroup branch from c861c62 to 0c07ce0 Compare May 15, 2024 07:23

ThomasBreuer mentioned this pull request May 15, 2024

ambiguous printing of sub-pc groups #3732

Closed

fingolfin approved these changes May 15, 2024

View reviewed changes

fingolfin merged commit c06c1b5 into oscar-system:master May 15, 2024
26 checks passed

ThomasBreuer deleted the TB_SubPcGroup branch May 15, 2024 11:39

fingolfin mentioned this pull request May 15, 2024

Add SubPcGroup type #2672

Closed

ThomasBreuer mentioned this pull request May 17, 2024

introduce SubFPGroup #3754

Merged

benlorenz mentioned this pull request Oct 16, 2024

booktest: add error checking for non-jlcon files also on master #4209

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

introduce `SubPcGroup` #3166

introduce `SubPcGroup` #3166

ThomasBreuer commented Jan 10, 2024

fingolfin commented Jan 22, 2024

fingolfin Jan 22, 2024

fingolfin May 13, 2024

ThomasBreuer May 15, 2024

fingolfin Jan 22, 2024

ThomasBreuer Jan 22, 2024

fingolfin Jan 22, 2024

fingolfin commented Jan 31, 2024

fingolfin commented Feb 28, 2024

ThomasBreuer commented May 2, 2024

ThomasBreuer commented May 3, 2024

benlorenz commented May 3, 2024

codecov bot commented May 4, 2024 •

edited

Loading

fingolfin left a comment

fingolfin May 13, 2024

fingolfin May 13, 2024

fingolfin commented May 14, 2024

ThomasBreuer commented May 15, 2024

fingolfin commented May 15, 2024

		# # Remove once Hecke lower bound is >= 0.14.1
		# @attributes MultTableGroup

introduce SubPcGroup #3166

introduce SubPcGroup #3166

Conversation

ThomasBreuer commented Jan 10, 2024

fingolfin commented Jan 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fingolfin commented Jan 31, 2024

fingolfin commented Feb 28, 2024

ThomasBreuer commented May 2, 2024

ThomasBreuer commented May 3, 2024

benlorenz commented May 3, 2024

codecov bot commented May 4, 2024 • edited Loading

Codecov Report

fingolfin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fingolfin commented May 14, 2024

ThomasBreuer commented May 15, 2024

fingolfin commented May 15, 2024

introduce `SubPcGroup` #3166

introduce `SubPcGroup` #3166

codecov bot commented May 4, 2024 •

edited

Loading