Questions and concerns regarding type GroupCoset

[fingolfin]

Right now, there is a single type GroupCoset which is used to both represent right cosets (the "native" GAP way) and left cosets. I have some concerns and questions about that:

1. given a GroupCoset there seems to be no good way to find out whether it is left or right; best I can do is directly access its side field, it seems better to have test methods like isleftcoset / isrightcoset

2. however, perhaps it would be better to have two types for that, LeftGroupCoset and RightGroupCoset (or just LeftCoset / RightCoset but then I am not sure if people want cosets in e.g. modules, leading to a potential conflict?). As a variant, the "side" could be part of the parametric type

3. Why is GroupCoset mutable?

4. Once we can distinguish left/right cosets, it would be nice to be able to multiply

   1. a right coset Hx by an element y to get Hxy
   2. y * xH = yxH
   3. Hx * yK = HxyK

5. Some internal documentation should be added, which explains what we do to "fake" left cosets, even though GAP only support right cosets

[GDeFranceschi]

To see whether a coset C it is left or right, it is sufficient to display it: if it's left, it is displayed as xH, otherwise as Hx. At machine level, the structure coset has the field side to that says whether the coset is left or right. If we want a function like isrightcoset, it should be easy to write it. Would it apply only to objects of type Coset, or to generic subsets of a group?

I don't remember why it is a mutable structure: it was probably part of an experiment, but as far as I see now, it doesn't need to be mutable, so I can modify it, and adding the function multiplication and the documentation.

[thofma]

I would suggest isleft/isright instead isleftcoset/isrightcoset.

[fingolfin]

@thofma could you elaborate as to why you think this is better? (honest question, I am not disagreeing, just wondering)

BTW, I note that we already have isbicoset, which can apply to either left or right cosets. But this is not something I'd encode in a type or so; just wanted to mention it for completness

[fingolfin]

@GDeFranceschi wrote:

To see whether a coset C it is left or right, it is sufficient to display it:

Yes, but that doesn't help me if I need to write code that has to distinguish the two, and doesn't allow for method dispatch either!

[thofma]

For the same reason we don't have isoddnumber and isevennumber or factor_polynomial and factor_number. It is kind of redundant and unnecessary in a language with method overloading/multiple dispatch. Here there is no need to put argument types into function names. Similar to an object oriented language, where one would probably prefer C.isleft() or C.isright().

It also prevents ducktyping, although in this case it probably won't matter.

[GDeFranceschi]

Some internal documentation should be added, which explains what we do to "fake" left cosets, even though GAP only support right cosets

Where do I document it? Just on the manual, or also on the docstring in the src file?

[fingolfin]

@GDeFranceschi in the docstrings, and then make sure the docstrings are included in the manual

[fingolfin]

Note that GAP doesn't really use RightCoset much, nor does it implement much functionality. So, I think we should perhaps consider not using it in our coset type. Instead, we can simply directly wrap a pair of a GAPGroup and a GAPGroupElem. I.e. something like this (pseudo code):

abstract type GroupCoset{T <: GAPGroup}
  G::T
  repr::GAPGroupElem{T}
  isleft::Bool
end

[ThomasBreuer]

Just now (when dealing with #3297) I stumbled over a problem with left cosets, which strengthens the above old comment by @fingolfin:

Currently a left coset in Oscar stores in its .X field a right coset in GAP, which means that several left cosets for the same subgroup in Oscar store GAP objects which belong to (conjugate but) different subgroups.
The == method for left cosets compares the underlying GAP objects, which means in the situation of different subgroups that GAP will compare these subgroups instead of just making one element membership test.

It would be much better to simply omit the .X field. Currently it is actually used only for the == method (were this is a bad idea) and in methods for iterate, rand, and intersect (where one can either create the GAP object on demand, or where one can better deal directly with the Oscar objects.)

(I think that this == method is a "nice" example how delegations can cause unexpected overhead.)

[fingolfin]

It would be fine by me to remove the .X field in cosets -- or to rename it and only lazily bind it in order to cache an auxiliary RightCoset object (the renaming would "hide" it from code that blindly assumes that any gapwrapperobject.X is a faithful representation of the content of the Julia object "wrapping" it). Of course e.g. intersect for two cosets Ua and Vb in a group G (or aU and bV!) is not quite trivial, but I guess that's one of the cases were one could create auxiliary RightCoset objects (and then either discard them or keep them for future use)

[ThomasBreuer]

Yes, that would be my proposal: Create group cosets in Oscar without an underlying GAP coset.
Create a GAP coset in those situations where it is useful, for example in intersect (the method in question is actually a bit more general than just an intersection of cosets).

[simonbrandhorst]

Most of this seems to have been addressed in #3899 .
Can this be closed?

[fingolfin]

I am OK with closing this. The one unresolved checkbox in my original list is bike shedding about types, and as long as it works... shrug or in other words: we can deal with it if and when we observe it to be an actual issue. Everything else indeed seems to be resolved.