Manual groups

[GDeFranceschi]

This branch has been created to write the manual for Groups (just another .md file in docs/src). But I just started.

[codecov]

Codecov Report

Merging #116 into master will decrease coverage by 0.03%.
The diff coverage is 33.33%.

@@            Coverage Diff             @@
##           master     #116      +/-   ##
==========================================
- Coverage   27.89%   27.85%   -0.04%     
==========================================
  Files          19       19              
  Lines        2811     2815       +4     
==========================================
  Hits          784      784              
- Misses       2027     2031       +4     

Impacted Files	Coverage Δ
src/Groups/homomorphisms.jl	80.18% <0.00%> (-2.23%)	⬇️
src/Groups/sub.jl	72.86% <0.00%> (-0.57%)	⬇️
src/Groups/GAPGroups.jl	78.60% <50.00%> (ø)
src/Groups/group_constructors.jl	88.57% <100.00%> (ø)

[fingolfin]

It has no memory of a parent? Does that make sense? Why is it called sub then, and not e.g. group?

[thofma]

Because it returns the embedding.

[GDeFranceschi]

I was thinking to create also a function called simply group, that works in a similar way to the GAP function Group.

[fingolfin]

@thofma I was only referring to the second variant (sorry, I failed to state that), which only takes a bunch of elements, not an existing group

@GDeFranceschi sounds good, that could replace the second sub variant

[thofma]

Yes, but even for the second variant it returns the new group H together with the embedding into G. Without the embedding, I would not call it subgroup, but just group.

[fingolfin]

Hmm.. I think this should be done differently: this text and the example should be part of the docstring for our methods for one, rand, gens. Then here, we should just "include" that documentation via @docs`

Suggested change

	* `one(G)` = the identity element of `G`.
	```@repl oscar
	one(symmetric_group(4))
	one(GL(2,3))
	```
	* `rand(G)`: returns a random element of the group `G`.
	For very large groups, the algorithm to ensure a uniform probability could be expensive, so there exists also the function `rand_pseudo`: this is much faster than `rand`, but the elements of `G` are not necessarily equally distributed.
	* `gens(G)`: returns a list of generators for the group `G`. To have access to the ``n``-th generator of the list, it is sufficient to type `G[n]` instead of `gens(G)[n]`.
	```@repl oscar
	G = symmetric_group(4);
	gens(G)
	G[1]
	```
	If the group `G` has been created as the subgroup of another group generated by a list of elements `L`, then the generating set returned by the function `gens` corresponds to `L`.
	!!! note
	The output of `gens(G)` is not, in general, the minimal list of generators for `G`.
	```@docs
	one
	rand
	gens
	```

Actually, that's probably not quite right yet, as Oscar.jl will include many one methods eventually, so we probably ought to tell it which one is meant here. Please carefully read https://juliadocs.github.io/Documenter.jl/stable/man/syntax/ it explains how to do this.

Perhaps also look at how some of our other packages do this; here are some select examples (you can easily find plenty more)

• https://github.com/oscar-system/GAP.jl/blob/master/docs/src/index.md
• https://github.com/Nemocas/AbstractAlgebra.jl/blob/master/docs/src/perm.md
• https://github.com/thofma/Hecke.jl/blob/master/docs/src/number_fields/elements.md
• https://github.com/Nemocas/Nemo.jl/blob/master/docs/src/polynomial.md
  Of course if you gens method has no good documentation comment right now, then you can take the text you wrote above and use that to form a doc comment instead.

[fingolfin]

I am not very fond of the @repl feature:

• it requires Julia to process all @repl examples just to be able to render the documentation, which means building the manual takes longer
• it misses an opportunity for further testing, as we never notice if Julia's output changes

Instead, please use @jldoctest (specifically, the REPL variant) as I suggested before; read https://juliadocs.github.io/Documenter.jl/stable/man/doctests/ for details

[fingolfin]

I don't think every permutation group is "identified" by a degree! Rather, every permutation group in Oscar has an associated degree.

The use of "ideally" renders this sentences rather vague. It implies that this "ideal" may not be always reached, but gives the reader no clue at all about when that is and so on. Also, is this really "ideal"? Perhaps you meant "usually"? What exactly do you want to convey here?

Perhaps more something like:

Suggested change

	Every permutation group `G` is identified by a degree, that corresponds ideally to the size of the set on which `G` is acting.
	In Oscar, every group has a parent (see section XYZ for details). For permutation groups, the parent currently always is a symmetric group. The degree `d` of a permutation group `G` then is the degree of its parent symmetric group. That is, `G` acts on the set `1..d`.

[GDeFranceschi]

Isn't parent defined only for group elements? The symmetric group is only a kind of "sup-group" for the permutation group G. Maybe we should say something like:
" Every permutation group G has a degree n, that is the cardinality of the set on which G acts."
Then add the note about deg(G) != largest moved point of G in general.

[fingolfin]

Suggested change

	The degree of a group of permutations is not necessarily the highest moved point of the group `G`. For example, if `G` is defined as a subgroup of `Sym(n)`, its degree is `n` even if every element of `G` fixes `n`.
	The degree of a permutation group is not necessarily equal to the largest moved point of the group `G`. For example, the trivial subgroup of `symmetric_group(d)` has degree `d` even tough it fixes `n`.

[fingolfin]

This should be inside the perm resp. cperm doc strings

[fingolfin]

This sounds like it should be inside the docstring of cperm, not here. Also, it is a bit unclear what the significance of this statement is? Should this behavior be surprising? If so, perhaps it should explained what happens in this case? Or perhaps it all works fine?

[GDeFranceschi]

It simply means that when the function gets in input non-disjoint cycles, it multiplies them instead of returning an error, like GAP and Magma do.
I just felt to write this line just because the behaviour is different from GAP.

[fingolfin]

Yeah, I guess I'd move this warning before the example, and also augment it by mentioning that this is why instead we recommend the notation n^x, as then (n^x)^y == n^(x*y)

And at that point we might also want to warn that in Julia a^b^c is evaluated as a^(b^c) (while in GAP it thankfully gives an error).

[GDeFranceschi]

I left the notation x(n) instead of n^x because x(n) is the syntax. Now I was thinking about it: do I also implement the function ^(n::Int)(x::Perm) having the same output as x(n)?

[fingolfin]

Suggested change

	The type need to be specified in the input of the function `abelian_group`, otherwise a group of type `GrpAbFinGen` is returned.
	The type need to be specified in the input of the function `abelian_group`, otherwise a group of type `GrpAbFinGen` is returned, which is not a GAP group type. In future versions of Oscar, this may change.

[fingolfin]

Suggested change

	Anyway, recall that variables named `f1`, `f2`, etc. are not initialized! To get the generators of the group `G`, one has to refer to them as `G[1]`, `G[2]`, etc.
	Note that this does not define Julia variables named `f1`, `f2`, etc.! To get the generators of the group `G`, use `gens(G)`; for convenience they can also be accessed as `G[1]`, `G[2]`, etc.

"Anyway" is colloquial. And "recall" doesn't seem to apply (from where should this be recalled?). Also note that the point about "there are no Julia variables with those names by default" also applies to free groups and in general all finitely presented groups (of which pc groups in a sense are a special case).

At the same time, access to the generators is something that really concerns all groups, so perhaps it would be better to move that into its own section, and then just insert a reference link here. (Please look into the Documenter.jl documentation to learn how to insert cross links to other section)

Finally, you could also mention the possibility of writing f1, f2 = gens(G).

[fingolfin]

Are you sure that an "ERROR is returned"? Isn't rather a exception thrown? Same for other sections with similar text

[GDeFranceschi]

For the permutation groups, an ArgumentError is returned. I'll check for similar parts.

[fingolfin]

Suggested change

	Return the permutation `x` sending `i` into `L[i]` for every `i`. `L` must contain exactly one time every integer from 1 to `n` for `n` = length(`L`).
	Return the permutation `x` which maps every `i` from `1` to `length(L)` to `L[i]`. `L` must contain every integer from 1 to `length(L)` exactly, otherwise an exception is thrown.

[rfourquet]

Couldn't these two functions be merged? It looks indeed confusing to have both in the same system.

[GDeFranceschi]

Two functions called haspreimage and has_preimage already exist in Hecke. The first one is defined on variables of types GrpAbFinGenMap and GrpAbFinGenElem, while the second is defined on variables of type GrpGenToGrpGenMor and GrpGenElem, and they do essentially the same thing. If we want to merge these two functions, this has to be done in the Hecke package first. Then, in Oscar, we can simply import the function we decide to preserve and extend it to GAPGroups.

[thofma]

This is just an oversight/inconsistency in Hecke. We should not replicate this here :)

We just need to decide for one convention and follow it through. There is no need to change it in Hecke first.