Skip to content
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

Groups: add minimal_generating_set #2816

Merged
merged 2 commits into from
Oct 4, 2023

Conversation

fingolfin
Copy link
Member

No description provided.

@codecov
Copy link

codecov bot commented Sep 19, 2023

Codecov Report

Merging #2816 (aff17b7) into master (add39f2) will decrease coverage by 0.01%.
Report is 2 commits behind head on master.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2816      +/-   ##
==========================================
- Coverage   80.68%   80.68%   -0.01%     
==========================================
  Files         456      456              
  Lines       64755    64762       +7     
==========================================
+ Hits        52250    52251       +1     
- Misses      12505    12511       +6     
Files Coverage Δ
src/Groups/GAPGroups.jl 94.28% <100.00%> (+0.09%) ⬆️

... and 7 files with indirect coverage changes

Copy link
Member

@ThomasBreuer ThomasBreuer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

The definition of rank does not agree with rank for GrpAbFinGen.
Perhaps the definition of is_free should be more explicit.
(See my comments.)

end
GAP.Globals.SetRankOfFreeGroup(G.X, n)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see: RankOfFreeGroup belongs to GAP's FGA package, therefore the function FreeGroup cannot set this attribute. Thus the situation here is better.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aye.

Regarding the definition of rank(::GrpAbFinGen) differing (it returns the Z-rank, or the "Hirsch length" to use polycyclic group terminology): ahh, that's a shame. Dang. Well, I could rename this function _rank for now, so that I can use it in the printing code (that was what motivated this PR), and then possibly again restrict it to support "free fp groups" only. And then worry about how to expose this to users later.

@doc raw"""
is_free(G::GAPGroup)

Return whether `G` is a free group.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should say what is meant with "is a free group".
GAP's IsFreeGroup means essentially a group created with free_group or a subgroup of such a group.
The GAP documentation is also vague about this, but it states that groups consisting of elements in the filter IsAssocWordWithInverse are regarded as free groups.

One could have the idea that "free group" means a group that is free on some set of elements, and then each trivial group is free on the empty set, and certain factors of free groups (for example where just some generators are mapped to the identity) are also free.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this important observation.

Adding to it, also any infinite cyclic group (e.g. represented as a pcp group) would be "free". And yeah, in GAP we have this mathematically surprising situation then:

gap> G:=AbelianGroup([0]);
<fp group of size infinity on the generators [ f1 ]>
gap> IsFreeGroup(G);
false
gap> IsFreeGroup(FreeGroup(1));
true

So this is a case were one needs to distinguish between "mathematically free" (on some unknown set) and "technically free" (created as a free group), for lack of better terminology.

Both are of independent interest.... But I think a function is_free for groups should aim to check for the mathematical property, though of course it cannot in general (as long as we can't generally decide whether a fp group is trivial, we also can't decide freeness). But we can do it in many other cases, e.g. for finite groups (where is_free(G) = is_trivial(G)), pcp groups (either trivial or infinite cyclic, both can be tested for efficiently). For fp groups, we could make a best effort: compute abelian invariants; if any are non-zero, return false; otherwise we can read of the potential rank of the "free" group (if it is free); then use TzGoGo to try and simply (a copy of) the presentation to see if it is "obviously" free (= no relations left).

But of course what I really am using is the "technical interpretation". I actually thought already that perhaps we should split the FPGroup type not just into FPGroup and SubFPGroup (as we already discussed at PR #2672), but in fact also have FreeGroup (and perhaps also SubFreeGroup). In that case the "technical" check becomes a type check.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think that the "technical interpretation" is used more frequently; also GAP's IsFreeGroup is just a filter, there was no aim to support a check for the mathematical property.

So do we need a better name for the "technical interpretation", in order to keep is_free for the mathematical one,
or do we decide that we introduce the type FreeGroup as mentioned above?

(I guess the name IsFreeGroup was chosen because people did not think that checking the mathematical property for arbitrary f. p. groups would be interesting. Moreover, as soon as such a function would be available, somebody would come and ask for a function that checks whether a group is free on a given set, and for a function that returns a set on which a given group is free ...)

"""
rank(G::GAPGroup)

Return the rank of the group `G`, i.e., the minimal size of a generating set.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GrpAbFinGen defines rank as the rank of the free part. Thus torsion groups created with abelian_group have always rank zero. If we really want to define rank for GAPGroup as the cardinality of a minimal generating set then perhaps we should mention this.

@fingolfin fingolfin changed the title Groups: add minimal_generating_set & rank Groups: add minimal_generating_set Sep 26, 2023
Also add disabled `rank` and `is_free` methods for groups, to be enabled
(or removed) in future updates
@fingolfin
Copy link
Member Author

In order to move forward, I have for now disabled the rank and is_free methods, to be discussed later. But minimal_generating_set is kept.

src/Groups/GAPGroups.jl Outdated Show resolved Hide resolved
Co-authored-by: Johannes Schmitt <[email protected]>
@fingolfin fingolfin merged commit 0c82301 into oscar-system:master Oct 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants