Oscar.jl icon indicating copy to clipboard operation
Oscar.jl copied to clipboard

`subgroups` command

Open fingolfin opened this issue 1 year ago • 0 comments

@wdecker just stumbled over this usability issue.

In short, the subgroups command is confusing because it does different things for "GAP groups" and abelian groups.

Consider:

help?> subgroups
search: subgroups psubgroups subgroup_reps hall_subgroups_representatives normal_subgroups minimal_subgroups

   subgroups(G::GrpGen; order::Int = 0,
                            index::Int = 0,
                            normal::Bool = false,
                            conjugacy_classes::Bool = false)

  Returns subgroups of G with embedding.

  ─────────────────────────────────────────────────────────────────

  subgroups(g::GrpAbFinGen;
            subtype = :all ,
            quotype = :all,
            index = -1,
            order = -1)

  Return an iterator for the subgroups of G of the specific form.

  ─────────────────────────────────────────────────────────────────

  subgroups(G::Group)

  Return the vector of all subgroups of G.

While technically the signatures suggest that not every group type supports all, cleaning this requires knowledge of what GrpAbFinGen and GrpGen are and are not.

The other issues is that the former two methods return iterators, where each entry is a subgroup together with an embedding, while the latter just returns a vector of subgroups (which technically is also iterable; but the "embedding" bit is missing).

Of course the two first methods are also inconsistent with what they support and do not support, and don't document the meaning of the various kwargs...

One way to improve this would be to at least somewhat align these, i.e.,

  • teach subgroups for GAPGroups to also at least suppor the index and order kwargs
  • maybe also conjugacy_classes and normal (after figuring out what they really do -- I have guesses but details matter)
  • return pairs of group, embedding after all (???) -- though I'd prefer if we instead finally implemented the canonical_map proposal which allows retrieving these later if needed (but of course I am heavily influenced here because for "my" groups and applications, I almost never need them).

Implementation could just be naive, i.e. "compute all subgroups and discard unwanted ones" -- hey, not ideal, but a start.

In any case, the GAP Groups docstring for subgroups should also contain a warning that suggests that in many cases it is better to use conjugacy_classes_subgroups.

A radical alternative would be to change the subgroups method for GAPGroups to take arbitrary kwargs, and then just do error("not supported for non-abelian groups, use XYZ instead").

fingolfin avatar Sep 21 '23 07:09 fingolfin