kalmarek / groupscore.jl Goto Github PK

From README:

Additionally the following assumptions are placed on the iteration:

• first(G::Group) must be the identity

It would be useful to be able to run the conformance tests for a group that does not implement the GroupsCore API. Maybe by using the generators ? Or Maybe by combining the generators randomly ?

Personally, I think that GroupElem should be used since AA uses that syntax.

What do we need to provide a useful interface for those? @thofma

I am wondering in how far this should be left at the "core" API, or whether we should also start to define a few more things? E.g.

• basic properties like isabelian, issolvable, isnilpotent, isperfect...
• standard subgroups like derivedsubgroup(G), center(G), socle(G), ... or sylowsubgroup(G, p)
• some subgroup related functionality like issubgroup(A,B), isnormalsubgroup(A,B), ...
• standard operations like centralizer(G, H), normalizer(G, H), normalizer(G, H), ...

This issue is used to trigger TagBot; feel free to unsubscribe.

If you haven't already, you should update your TagBot.yml to include issue comment triggers.
Please see this post on Discourse for instructions and more details.

If you'd like for me to do this for you, comment TagBot fix on this issue.
I'll open a PR within a few hours, please be patient!

As a first step in using the GroupsCore conformance tests for Oscar groups, I have tried the following in a Julia session.

using Oscar
using GroupsCore
using Test
include(pathof(GroupsCore) * "/../../test/conformance_test.jl");
G = symmetric_group(3);
test_Group_interface(G)

(It may be necessary to change the AbstractAlgebra version in GroupsCore's dependencies, depending on the Oscar version.)

I get the one failure and two kinds of errors.

• The failure happens in the test order(G) isa Integer; this is due to the fact that the unary Oscar.order returns an fmpz, which is not an integer. (Well, it is not a Base.Integer. Note that Oscar.Integer is set to fmpz, thus one has to be careful about the context of the code.)
  Thus the type check itself is perhaps not what one wants; one could add the desired type as an argument to test_Group_interface. I do not see how to fix the failure on the Oscar side.

• The first error happens in the statement order(Int16, G), I get MethodError: order(::Type{Int16}, ::PermGroup) is ambiguous.
  This is due to the fact that Julia regards the order methods in GroupsCore.jl/src/groups.jl and in Oscar.jl/src/Groups/GAPGroups.jl as "concurrent". Note that the GroupsCore installation is for order(::Type{<:Integer}, G::Group) and the Oscar installation is for order(::Type{T}, x::Union{GAPGroupElem, GAPGroup}) where T. So we could fix the problem in Oscar by installing the method also for order(::Type{T}, x::GAPGroup) where T<:Base.Integer, but this looks ugly; is there a better way?

• The other errors happen in calls of the form GroupsCore.pseudo_rand(G), which delegate to rand(Random.default_rng(), G). However, the unary rand method for a GAP group in Oscar delegates to a GAP function that uses GAP's random number generators. Is it really necessary to support Random.default_rng()?
  (We have GroupsCore.pseudo_rand and Oscar.rand_pseudo, this may be confusing.)

Option to discuss:

• comparing orders and numbers
• no arithmetics (promotion rules), you need to convert to a number explicitly

e.g.
https://gist.github.com/kalmarek/640630a53d5837a5c7fe401eb2f047bd

• Forcing everybody to implement deepcopy_internal, even for group elements with singleton (or cached) parents. Here Base.deepcopy should work just fine (fixed by #50)
• how to tell people to implement random API without forcing them to go the trivial sampler route? (fixed by #50)

in a few places I added

!!! note
    This function may not return, due to unsolvable word problem.

to the docstrings. Consequently, this note should be placed everywhere (every function that uses ==, or isone, order etc.). See #13 (comment)

What could be done not to clutter the documentation?
@fingolfin

using https://github.com/JuliaDocs/Documenter.jl.

@albinahlback would you give it a shot?

Hi @kalmarek thank you very much for working on this. Some quick remarks from looking at the README only (so if the code or docstrings would have made things clearer, I missed that, but I didn't find a link to a generated version of the manual; is there one? perhaps add a badge for it in the README?):

• It says "If finiteness can not be easily established one needs to override the default ..." and then talks about the default being "IteratorSize = Base.HasLength" -- but doesn't that default actually indicate that the default is that groups of a given type are all assumed to be finite and with order small enough to fit into an Int?? I'd really suggest to make the default SizeUnknown, and perhaps we can add a "trait" which makes it easy to specify that all instances of a given type are finite (but even then, it would mean that I can't use the iterator interface to get, say, the first 100 elements of Sym(100)). If you have good arguments why the default should really be HasLength, let's at least say something more like this (in spirit; the wording I use here probably is crap): "By default, HasLength is used which means that iteration will work for finite groups of your type, but will result in an error if the group has more than typemax(Int) elements"

• from reading this text, I suspect we have divergent ideas about what hasgens(G) == true should mean: for me (coming from the GAP background) it means "generators of this group are already known / they are already set" (so it could also be called knowsgens(G); for you it seems to mean "generators are known or computable" (i.e. cancomputegens(G)) . At least that's how I interpret the explanation for GroupsCore.gens.... I any case, we need to agree on the intended meaning (possibly splitting the function into two or more, as needed), and carefully document this

• BTW I am mostly open on the has vs knows vs ... discussion, but it ideally should be decided globally for all of OSCAR (this may very well require changing code). That said, I am happy to use one set of names now even if it means we need to change it down the road (i.e., IMHO we don't need to settle this with finality before we can proceed). Let me add, however, that for GAP, it was also useful to have a generic way to map from a "getter" to a "tester": i.e. given the "getter" function IsFinite, in GAP one can call Tester(IsFinite) and it will return the "tester" function HasIsFinite (which determines whether GAP already "knows" whether the group is finite. I.e., whether is information is cached or not)

• what is the idea behind istrulyequal and ==? I am rather reluctant to allow use of == for non-mathematical equality... Could we perhaps instead use == for math equality and Julia's isequal for "cheap" comparison? Maybe not, depends on what your intention here were...

• for groups, I think we also need an isfinite function (and perhaps also a hasisfinite / knowsisfinite / knows(:isfinite) / ... function)

• for group elements, again, hasorder for me would indicate knowsorder and for you (I think) means "order is finite and can be computed... how about isfiniteorder or hasfiniteorder or so?

• deepcopy is not yet implemented for GapObj from GAP.jl (see this issue), and implementing it in full generality is a bit tricky, but that doesn't mean it can't be implemented immediately for at least some cases (e.g. permutations); we should do this in GAP.jl ASAP, and then throw an error for cases which are not yet covered (which is infinitely better than what we do now, namely "crash"...)

• order should specify what happens if it is invoked on an infinite group / element of infinite order

• Option 1: we "implement" only === and isisomorphic; == throws an error.
• Option 2: what == should mean?

I suppose that additative groups is alias for commutative groups. Since AA implements this abstract type, I think we should as well.

Some comments, like

@doc Markdown.doc"""
     order(...)
...

Only arbitrary size integers are required to return mathematically correct answer.
"""

I think is a better fit for the readme, alternatively the documentation.

What do you think about this?

We are trying to steer away from (domain-specific) abbreviations in Oscar (see the recent deprecations of lm, lc and coeff*). I think it would be good idea to take this also into account here right from the beginning. So instead of comm it would be commutator or conjugate instead of conj.

Slogan:

The modification of the result of gens(G) shall not modify the parent object.

Several functions take two or more groups as arguments; typically a group and a (candidate) subgroup thereof. We should try to establish rules (as simple as possible) for this to ensure consistency and make it possible for users to understand code without having to always double check these orders.

In GAP, the rule is rather simple indeed: arguments are ordered by "size", where "size" is meant figuratively, not literally: First the "big" group, then the "subgroup" (resp. candidate for being a subgroup). So it is index(grp, sub), quo(grp, sub), issubgroup(grp, sub), isnormal(grp, sub), ...

However, in Julia, this is problematic: there, functions like issubset use the reverse order: issubset(subset, full) -- we discussed this further at oscar-system/Oscar.jl#163