WIP: next step for documentation of groups #583

ThomasBreuer · 2021-07-27T22:30:46Z

changed some docstrings
changed the code where it is necessary for documenting it
(e.g., admit a type as optional first argument for exponent, index)
export ischaracteristic not ischaracteristic_subgroup
(the latter is not needed)
extended and adjusted tests

fingolfin · 2021-07-28T08:19:21Z

src/Groups/GAPGroups.jl


-Return the length of the array gens(G).
+Return the length of the array `gens(G)`, see [`gens`](@ref).


Would this work?

Suggested change

Return the length of the array `gens(G)`, see [`gens`](@ref).

Return the length of the array [`gens`](@ref)`(G)`.

fingolfin · 2021-07-28T09:09:48Z

src/Groups/GAPGroups.jl

-Return the nilpotency class of `G`, that is the smallest integer `d` such that `G` has a central series of length `n`.
+Return the nilpotency class of `G`, that is, the smallest integer `d` such that `G` has a central series of length `n`.
+
+An error is signalled if `G` is not nilpotent.


Suggested change

An error is signalled if `G` is not nilpotent.

An exception is thrown if `G` is not nilpotent.

fingolfin · 2021-07-28T09:20:14Z

src/Groups/group_constructors.jl

- `free_group(L::String...) -> FPGroup`; return the free group with length(`L`) generators, printed as `L[1]`, `L[2]`, `L[3]`, etc.
- `free_group(L::Array{String,1}) -> FPGroup`; same as above.
- `free_group(n::Int, s::String) -> FPGroup`; return the free group of rank `n`, with generators printed as `"s1"`, `"s2"`, `"s3"`, etc.
+The second form, if there are `n` arguments `L...`,


Maybe this was meant?

Suggested change

The second form, if there are `n` arguments `L...`,

The third form, if there are `n` arguments `L...`,

fingolfin · 2021-07-28T09:23:46Z

src/Groups/GAPGroups.jl


-order(x::Union{GAPGroupElem, GAPGroup}) = order(fmpz, x)
+"""
+    order([::Type{T}, ]x::Union{GAPGroupElem, GAPGroup}) where T


This is inspired by GAPDoc convention, where we signal that the first argument is optional. Question is whether this is clear for other people. @fieker @thofma and others: thoughts?

An alternative would be to imiate the Julia convention for default values:

Suggested change

order([::Type{T}, ]x::Union{GAPGroupElem, GAPGroup}) where T

order(::Type{T} = fmpz, x::Union{GAPGroupElem, GAPGroup}) where T

Third alternative would be to list both signatures:

Suggested change

order([::Type{T}, ]x::Union{GAPGroupElem, GAPGroup}) where T

order(::Type{T}, x::Union{GAPGroupElem, GAPGroup}) where T

order(x::Union{GAPGroupElem, GAPGroup})

According to https://docs.julialang.org/en/v1/manual/documentation/ (first item), [ ... ] is used for optional arguments without default value?

Yes. Thus the first of the two above alternatives fits (and also has the advantage that the default need not be explained in the text).

fingolfin · 2021-07-28T09:24:35Z

src/Groups/GAPGroups.jl


-order(x::Union{GAPGroupElem, GAPGroup}) = order(fmpz, x)
+"""
+    order([::Type{T}, ]x::Union{GAPGroupElem, GAPGroup}) where T


Orthogonal to all this: should we restrict T to be Union{Integer, fmpz} ? And: do we already have a type alias for this probably very common union?

fingolfin · 2021-07-28T09:33:43Z

src/Groups/libraries/smallgroups.jl


+#T Do we want this???


I will partially deal with this in my PR for the group libraries, by turning this into an iterator.

fingolfin · 2021-08-09T00:02:00Z

@ThomasBreuer What is the status of this? I am somewhat surprised to see that it's still not complete after almost two weeks? What's the holdup?

(Also, now it needs to be rebased...)

ThomasBreuer · 2021-08-09T21:01:19Z

@fingolfin Now it is ready for review. Sorry for the delay.

fingolfin

Thanks! Some more minor comments, but this is basically mergeable already.

fingolfin · 2021-08-09T21:56:55Z

src/Groups/GAPGroups.jl

 """
    rand_pseudo(G::Group)

-Return a random element of the group `G`. It works faster than `rand`, but the elements are not necessarily equally distributed.
+Return a pseudo random element of `G`.  This works faster than `rand`,
+but the returned elements are not necessarily equally distributed.


Suggested change

but the returned elements are not necessarily equally distributed.

but the returned elements are not necessarily uniformly distributed.

fingolfin · 2021-08-09T21:58:17Z

src/Groups/GAPGroups.jl

-function ==(x::PermGroupElem, y::PermGroupElem)
-   return x.X == y.X && degree(parent(x))==degree(parent(y))
-end
+==(x::PermGroupElem, y::PermGroupElem) = x.X == y.X && degree(parent(x))==degree(parent(y))


I realize you are just refactoring the existing code, but while we are at it: perhaps we should first check the degree (as it ought to be fast to get) before comparing the permutations? I.e.,

Suggested change

==(x::PermGroupElem, y::PermGroupElem) = x.X == y.X && degree(parent(x))==degree(parent(y))

==(x::PermGroupElem, y::PermGroupElem) = degree(parent(x))==degree(parent(y)) && x.X == y.X

fingolfin · 2021-08-09T22:00:20Z

src/Groups/GAPGroups.jl

+At the moment, the input vectors of the function `cperm` need not be disjoint.
+
+!!! warning
+    If the function `perm` is evaluated in a vector of integers without specifying the group `G`, then the returned value is an element of the AbstractAlgebra.jl type `Perm{Int}`. For this reason, if one wants a permutation of type `GAPGroupElem{PermGroup}` without specifying a parent, one has to use the function `gap_perm`.


Very long line, maybe wrap it?

fingolfin · 2021-08-09T22:03:56Z

src/Groups/GAPGroups.jl


-Return a Hall `P`-subgroup of `G`. It works only if `G` is solvable.
+Return a Hall `P`-subgroup of `G`.


A definition of "Hall subgroup" would be nice, possibly taken from the GAP manual. Likewise for an example (again we could borrow from the GAP manual).

fingolfin · 2021-08-09T22:05:55Z

src/Groups/GAPGroups.jl


-Return a Hall `P`-subgroup of `G`. It works only if `G` is solvable.
+Return a Hall `P`-subgroup of `G`.
+An exception is thrown if `G` is not solvable.


This indeed describes what the code does, but... why actually does it? In GAP, I think we support non-solvable groups, too, at least if they have suitable subgroups? Perhaps we don't want to support this here -- fine, but then we should perhaps still elaborate why, i.e. why we throw an exception here?

@fingolfin The GAP function HallSubgroup works also for (finite) nonsolvable groups, and returns either a Hall P-subgroup (if it is unique up to conjugacy), or a list of Hall P-subgroups (representatives of G-conjugacy classes of these subgroups if there is more than one class), or fail (if no Hall P-subgroup exists).

For Oscar, we could introduce a function representatives_hall_subgroups, which always returns a vector of representatives of G-conjugacy classes of Hall P-subgroups.

fingolfin · 2021-08-09T22:11:28Z

src/Groups/group_constructors.jl

+Here, `T` must be one of `PermGroup`, `FPGroup`, or `PcGroup`.
+
+!!! warning
+    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.


Line wrap?

Also, of course we need to properly integrate the abelian groups provided by Hecke.... but that's another PR

fingolfin · 2021-08-09T22:12:05Z

src/Groups/group_constructors.jl

 """
 function abelian_group(::Type{T}, v::Vector{Int}) where T <: GAPGroup
-  v1 = GAP.julia_to_gap(v)
-  return T(GAP.Globals.AbelianGroup(_gap_filter(T), v1))
+  return T(GAP.Globals.AbelianGroup(_gap_filter(T), GAP.julia_to_gap(v)))


Assuming it works (I didn't test), generally I find this nicer / more "Julia-ish":

Suggested change

return T(GAP.Globals.AbelianGroup(_gap_filter(T), GAP.julia_to_gap(v)))

return T(GAP.Globals.AbelianGroup(_gap_filter(T), GAP.GapObj(v)))

Yes. (I had already replaced several occurrences.)

fingolfin · 2021-08-09T22:13:25Z

src/Groups/libraries/primitivegroups.jl

-Return the list of all primitive groups (up to isomorphism) satisfying the conditions in `L`. Here, `L` is a vector whose arguments are organized as `L` = [ `func1`, `arg1`, `func2`, `arg2`, ... ], and the function returns all the groups `G` satisfying the conditions `func1`(`G`) = `arg1`, `func2`(`G`) = `arg2`, etc. An argument can be omitted if it corresponds to the boolean value ``true``.
+Return the list of all primitive groups (up to permutation isomorphism)
+satisfying the conditions in `L`.
+Here, `L` is a vector whose arguments are organized as `L` = [ `func1`, `arg1`, `func2`, `arg2`, ... ], and the function returns all the groups `G` satisfying the conditions `func1`(`G`) = `arg1`, `func2`(`G`) = `arg2`, etc. An argument can be omitted if it corresponds to the boolean value `true`.


Wrap this line?

fingolfin · 2021-08-09T22:13:48Z

src/Groups/libraries/smallgroups.jl

@@ -13,10 +13,10 @@ export
 """
    small_group(n::Int, i::Int)

-Return the `i`-th group of order `n` in the catalogue of the GAP Small Groups Library. The group is given of type ``PcGroup`` if the group is solvable, ``PermGroup`` otherwise.
+Return the `i`-th group of order `n` in the catalogue of the GAP Small Groups Library. The group is given of type `PcGroup` if the group is solvable, `PermGroup` otherwise.


fingolfin · 2021-08-09T22:15:17Z

src/Oscar.jl

@@ -87,6 +87,7 @@ function __init__()
        (GAP.Globals.IsFpGroup, FPGroup),
    ])
    GAP.Packages.load("forms")
+    GAP.Packages.load("ctbllib")


What's this about? Is this preparation for moving your character table code into the "official" part of Oscar.jl (you are still working on that, right?)

Yes, this addition accidentally slipped in ...

rfourquet · 2021-08-10T07:42:24Z

src/Groups/sub.jl

@@ -248,21 +254,26 @@ end
 """
    issolvable(G::GAPGroup)

-Return whether the `G` is solvable.
+Return whether `G` is solvable,
+i. e., whether [`derived_series`](@ref)(`G`)


The simpler following version should also work:

Suggested change

i. e., whether [`derived_series`](@ref)(`G`)

i. e., whether [`derived_series(G)`](@ref)

@rfourquet Yes, this works, thanks. However, I prefer the "more involved" variant because it creates a hyperlink on "derived_series", whereas your variant creates a hyperlink on "derived_series(G)".

rfourquet · 2021-08-10T07:47:02Z

src/Groups/GAPGroups.jl


-Return the length of the array gens(G).
+Return the length of the array [`gens`](@ref)`(G)`.


Suggested change

Return the length of the array [`gens`](@ref)`(G)`.

Return the length of the array [`gens(G)`](@ref).

src/Groups/GAPGroups.jl

fingolfin · 2021-08-11T22:59:08Z

src/Groups/GAPGroups.jl

-Return (``true``,nothing) if `G` is the trivial group, (``true``,``p``) if |`G`| is a non-trivial ``p``-power, (``false``,nothing) otherwise.
+Return `(true, nothing)` if `G` is the trivial group,
+`(true, p)` if |`G`| is a non-trivial power of a prime `p`,
+and `(false, nothing)` otherwise.


This is potentially inaccurate, as in current GAP versions, IsPGroup may also return true for infinite p-groups (not that any are available in GAP, as far as I know...)

fingolfin · 2021-08-11T23:02:00Z

test/Groups/constructors.jl

            else
-           #    @test index(GO(e,n,q),omega_group(e,n,q))==2
+               @test index(GO(e,n,q),omega_group(e,n,q))==2


Here's a conflict now... please resolve, then merge this

- fixed a few `@docs` blocks in `docs` files such that the correct docstrings are specified - moved text from `docs` files to `src` files where appropriate - made statements about exceptions uniform (say "An exception is thrown if ...") - used `@gapattribute` in more cases - added/extended/corrected docstrings - changed the syntax in lines that show the signatures of the functions such that default values are shown, and removed the related (now unnecessary) sentences from the text. - use LaTeX markup for math mode where appropriate

@fingolfin

(as suggested by @fingolfin), and rebased

ThomasBreuer · 2021-08-12T14:16:10Z

The test with Julia 1.7 on ubuntu ran out of time and got cancelled. I restart the tests.

ThomasBreuer · 2021-08-12T15:25:52Z

Now the tests passed (except for the nightly tests), within about one hour.

fingolfin reviewed Jul 28, 2021

View reviewed changes

fingolfin mentioned this pull request Jul 29, 2021

access to GAP's "tester" and "setter" functions #431

Merged

fingolfin reviewed Aug 9, 2021

View reviewed changes

rfourquet reviewed Aug 10, 2021

View reviewed changes

fingolfin approved these changes Aug 11, 2021

View reviewed changes

fingolfin reviewed Aug 11, 2021

View reviewed changes

ThomasBreuer added 3 commits August 12, 2021 09:45

address @fingolfin's suggestions

d62365f

fixed the definition if ispgroup

a3ff0f2

(as suggested by @fingolfin), and rebased

ThomasBreuer merged commit 5a4f12e into oscar-system:master Aug 12, 2021

ThomasBreuer deleted the TB_Groups_Documentation2 branch August 12, 2021 15:27


		Return the length of the array gens(G).
		Return the length of the array `gens(G)`, see [`gens`](@ref).

	Return the length of the array `gens(G)`, see [`gens`](@ref).
	Return the length of the array [`gens`](@ref)`(G)`.

	An error is signalled if `G` is not nilpotent.
	An exception is thrown if `G` is not nilpotent.

	The second form, if there are `n` arguments `L...`,
	The third form, if there are `n` arguments `L...`,

	order([::Type{T}, ]x::Union{GAPGroupElem, GAPGroup}) where T
	order(::Type{T} = fmpz, x::Union{GAPGroupElem, GAPGroup}) where T

	order([::Type{T}, ]x::Union{GAPGroupElem, GAPGroup}) where T
	order(::Type{T}, x::Union{GAPGroupElem, GAPGroup}) where T
	order(x::Union{GAPGroupElem, GAPGroup})

	but the returned elements are not necessarily equally distributed.
	but the returned elements are not necessarily uniformly distributed.

	==(x::PermGroupElem, y::PermGroupElem) = x.X == y.X && degree(parent(x))==degree(parent(y))
	==(x::PermGroupElem, y::PermGroupElem) = degree(parent(x))==degree(parent(y)) && x.X == y.X


		Return a Hall `P`-subgroup of `G`. It works only if `G` is solvable.
		Return a Hall `P`-subgroup of `G`.

	return T(GAP.Globals.AbelianGroup(_gap_filter(T), GAP.julia_to_gap(v)))
	return T(GAP.Globals.AbelianGroup(_gap_filter(T), GAP.GapObj(v)))

	i. e., whether [`derived_series`](@ref)(`G`)
	i. e., whether [`derived_series(G)`](@ref)

	Return the length of the array [`gens`](@ref)`(G)`.
	Return the length of the array [`gens(G)`](@ref).

WIP: next step for documentation of groups #583

WIP: next step for documentation of groups #583

Conversation

ThomasBreuer commented Jul 27, 2021

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

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 Aug 9, 2021

ThomasBreuer commented Aug 9, 2021

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

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

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

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

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

ThomasBreuer commented Aug 12, 2021

ThomasBreuer commented Aug 12, 2021