Add encode and pc_group for encoding finite pc groups into integers and back#5456
Conversation
|
the new changes that I made are probably too clunky and overloaded, please let me know if they're right if not what's the correct approach. |
Could you clarify what you mean by that? |
| H = isa(code, Int) ? pcgroup_code(code, G) : pcgroup_code(code) | ||
|
|
||
| # Test that the original and reconstructed groups are isomorphic | ||
| @test is_isomorphic(G, H) |
There was a problem hiding this comment.
Is the return value H of pcgroup_code only guaranteed to be isomorphic with the group G from which the code was computed, or is H in fact guaranteed to have the same polycyclic presentation as G?
In the latter case, we could check that hom(G, H, gens(H)) does not throw an error, which is both stronger and cheaper.
If we document this stronger property then this property can be mentioned as a reason why code_pcgroup is not defined for groups of type SubPcGroup: The generators list of a SubPcGroup is in general not a polycyclic generating sequence, and GAP's CodePcGroup is defined w.r.t. such a sequence for its input.
There was a problem hiding this comment.
Yes the same polycyclic presentation -- so, good point! well, we also should then add order(G) == order(H) to make sure we don't get a proper quotient.
And also good point about SubPcGroup.
There was a problem hiding this comment.
Although.. GAP does support this on subgroups of pc groups, it delegates to CodePcgs( Pcgs( G ) ) -- so from that vantage point, it would be good to also support it. The hom test would still work though, wouldn't it?
There was a problem hiding this comment.
Thank you for the clarification, I'll update my tests according to all the suggestions made. I will also document the restricted support for now.
There was a problem hiding this comment.
In general not.
julia> g = small_group(24, 12);
julia> s, _ = pcore(g, 2)
(Sub-pc group of order 4, Hom: s -> g)
julia> gens(s)
3-element Vector{SubPcGroupElem}:
f3
f4
f3*f4
julia> GAP.Globals.Pcgs(GapObj(s))
GAP: Pcgs([ f3, f4 ])
The code for s will be computed w.r.t. the pcgs of s, and the PcGroup created from the code will have a pcgs as its gens value.
Then hom will be asked to map the 3-element gens(s) to the 2-element gens value of the new group.
I think supporting code_pcgroup for SubPcGroups would be asking for trouble.
It is safer to request explicitly switching from a given SubPcGroup to a PcGroup before creating the code, because this is then the group whose presentation will correspond to that of the group created from the code.
(The GAP documentation of CodePcGroup and PcGroupCode should also be made more precise.)
|
hi, sorry if I don't respond for a day. I have a flight in an hour, I see a lot of tests are failing, I will get to them immediately as soon as I land in Germany tomorrow. Apologies for the delay. |
|
This PR has a lot of open comments. @varuntrehan7 please have a look at all of them again, and if you think that you did that change in a commit, click on "resolve conversation". That way we (and in particular you) can more easily see which comments you missed. |
|
Meta-question to @fingolfin and @ThomasBreuer: I don't quite like the names of the two new functions; they don't seem to fit in the OSCAR naming scheme. To me, a more appropriate name for the direction "(code, order) -> group" would be @varuntrehan7 please do not attempt to do any renamings here, until we have resolved the naming discussion! |
| ``` | ||
| """ | ||
| function pcgroup_code(code::IntegerUnion, order::IntegerUnion) | ||
| return PcGroup(GAP.Globals.PcGroupCode(GAP.GapInt(BigInt(code)),GAP.GapInt(BigInt(order)))) |
There was a problem hiding this comment.
As @lgoettgens pointed out this is inefficient. We will add a GAP.GapInt constructor in GAP.jl so that one can directly write GAP.GapInt(code)
There was a problem hiding this comment.
See oscar-system/GAP.jl#1283. Once that and oscar-system/GAP.jl#1284 are merged (and further 20 minutes have passed), this line can be changed to
| return PcGroup(GAP.Globals.PcGroupCode(GAP.GapInt(BigInt(code)),GAP.GapInt(BigInt(order)))) | |
| return PcGroup(GAP.Globals.PcGroupCode(GapInt(code), GapInt(order))) |
At the same time, please change
Line 38 in c0fadd1
There was a problem hiding this comment.
GAP.jl 0.16.1 is out now, so this can be done.
| pc_group(order::IntegerUnion, code::IntegerUnion) | ||
|
|
||
| Given an integer `order` and an integer `code`, return the polycyclic group it encodes. | ||
| Both `order` and `code` can be of type `Int`, `BigInt`, or `ZZRingElem`. |
There was a problem hiding this comment.
| Both `order` and `code` can be of type `Int`, `BigInt`, or `ZZRingElem`. |
fingolfin
changed the title
encode and pc_group for encoding finite pc groups into integers and back
|
@varuntrehan7 If you think that this is finished and CI passes (apart from any "nightly" and the "Enforce PR labels" jobs), please mark this PR as "ready to review" to get a final round of reviews. |
ThomasBreuer
left a comment
ThomasBreuer
left a comment
There was a problem hiding this comment.
encode and decode should eventually appear in the manual, for example in the section "Functions for (subgroups of) pc groups".
|
@varuntrehan7 this still needs to be hooked up in the |
fingolfin
added
the
release notes: use title
Hi, I emailed you a few days ago. I made the changes in the file, but to implement I need to create a new pr? I was confused because on top of it said it will automatically merge. Please let me know how to proceed, thankyou. |
|
Just push the changes here. |
| end | ||
|
|
||
| """ | ||
| pc_group(order::IntegerUnion, code::IntegerUnion) |
There was a problem hiding this comment.
The indentation here must be exactly four spaces (currently there are three). That's because a docstring is parsed as markdown, and four space indentation indicates a code block, i.e., that this line should be formatted like source code, not like english text.
| # GAP wrappers for group encoding / decoding | ||
|
|
||
| """ | ||
| encode(G::PcGroup) |
There was a problem hiding this comment.
also needs 4 instead of 3 spaces
| Compat = "4.13.0" | ||
| Distributed = "1.6" | ||
| GAP = "0.16.0" | ||
| GAP = "0.16.1" |
There was a problem hiding this comment.
| GAP = "0.16.1" | |
| GAP = "0.16.1" |
|
I don't know why the CI tests are red, because the test logs won't download for me. |
They are from last Friday, so I would assume it has to do with RPTU (and thus the runner) being down. |
5 participants
I've added functions to wrap CodePcGroup and PcGroupCode and the doctoring,testsets with respect to it. let me know for any corrections, also unsure where I need to import so for the time being they're inside my testset