Add letters function for PcGroupElem
#4202
Conversation
Moved from the mjrodgers-OW_GModules branch.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4202 +/- ##
========================================
Coverage 84.52% 84.52%
========================================
Files 646 645 -1
Lines 85631 85743 +112
========================================
+ Hits 72382 72477 +95
- Misses 13249 13266 +17
|
|
Thanks @jamesnohilly to clarify: what you posted in the description is not (yet) what you did, but a description of the work programme, right? Also, should mention that the code here was taken from PR #4108 by @fieker and is only the starting point. |
|
Yes, that's correct! I've now updated the description to make it clearer and also included a note that the code originates from PR #4108 as the starting point. |
There was a problem hiding this comment.
some comments. Maybe more directed towards @fingolfin and @ThomasBreuer instead of @jamesnohilly
|
@jamesnohilly tests are still failing. are on onto that, or do you need assistance? |
src/Groups/pcgroup.jl
Outdated
| julia> gg = pc_group(c) | ||
| Pc group of order 6 | ||
|
|
||
| julia> syllables(gg[1]^5*gg[2]^-4) |
There was a problem hiding this comment.
| julia> syllables(gg[1]^5*gg[2]^-4) | |
| julia> x = gg[1]^5*gg[2]^-4 | |
| f1*f2^2 | |
| julia> syllables(x) |
|
|
||
| # Convert syllables in canonical form into group element | ||
| #Thomas | ||
| function (G::PcGroup)(sylls::Vector{Pair{Int64, ZZRingElem}}; check::Bool=true) |
There was a problem hiding this comment.
Can you also add a similar constructor which takes an exponent vector, i.e., an inverse to letters?
There was a problem hiding this comment.
One needs to watch out for the semantic difference to the already existing function for FPGroups in
Oscar.jl/src/Groups/GAPGroups.jl
Line 2348 in 24711ee
There was a problem hiding this comment.
ugh, OK. perhaps we should kill that (is it documented?) first then. In GAP it made some sense to use such a flat list to avoid memory, as there are no tuples in GAP, only lists. But in Julia there is no real benefit of this over a Vector{Pair{Int64, ZZRingElem}}.
But that is way beyond this PR. So let's leave out the constructor I mentioned.
There was a problem hiding this comment.
No, not documented, but used for serialization. I haven't looked into how it is used there, so maybe we can just adapt the deserialization function, in the worst case it needs an upgrade script.
There was a problem hiding this comment.
ugh, OK. perhaps we should kill that (is it documented?) first then.
maybe @ThomasBreuer can look into that?
| letters(g::Union{PcGroupElem, SubPcGroupElem}) | ||
|
|
||
| Return the letters of `g` as a list of integers, each entry corresponding to | ||
| a group generator. |
There was a problem hiding this comment.
Note that we can also produce negative numbers: e.g. -3 means "inverse of 3rd generator". This should be explained, and perhaps an example added showing that. E.g. based on this:
julia> x = (gg[1]*gg[2]*gg[3])^-2
g1*g2^-2*g3^3
Perhaps also add something like this (and then mirror it in the other function)
See also [`syllables`](@ref).
There was a problem hiding this comment.
I have added a small example with some brief explanation to letters for this. However I am unsure if the example is good as I was not able to get elements with negative exponents and test.
There was a problem hiding this comment.
For that you need an infinite group. E.g.
julia> g = dihedral_group(PosInf())
Pc group of infinite order
julia> g[1]^-3 * g[2]^-3
g1*g2^-3
or
julia> g = abelian_group(PcGroup, [5, 0])
Pc group of infinite order
julia> g[1]^-3 * g[2]^-3
g1^2*g2^-3
|
This PR now unfortunately has conflicts with #4157. |
|
@jamesnohilly please try to resolve the conflicts. If you run into troubles (this is something that causes pain to many people esp. at the beginning) drop by my office and we'll figure it out together (ideally with your laptop if you have one) |
|
I've now merged the changes from #4157 with the changes made in this PR. From a quick look it seems to have merged without any major issues. |
As a starting point for this PR, I've borrowed the
syllablefunctions for PcGroup from PR #4108 to use as a reference for planned work.Planned work for this PR:
lettersfor (Sub)PcGroupElem._exponent_vectorfor whether the input is really "canonical".test/Groups/pcgroups.jlfor the new functions.Suggestions from @fingolfin