Add `letters` function for PcGroupElem #4202

jamesnohilly · 2024-10-14T14:13:54Z

As a starting point for this PR, I've borrowed the syllable functions for PcGroup from PR #4108 to use as a reference for planned work.

Planned work for this PR:

New function letters for (Sub)PcGroupElem.
A check in _exponent_vector for whether the input is really "canonical".
Test cases in test/Groups/pcgroups.jl for the new functions.
Add doc strings for new functions.

Suggestions from @fingolfin

Moved from the mjrodgers-OW_GModules branch.

codecov · 2024-10-14T15:29:38Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.60%. Comparing base (da7acb0) to head (1b80584).
Report is 50 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff            @@
##           master    #4202    +/-   ##
========================================
  Coverage   84.59%   84.60%            
========================================
  Files         631      640     +9     
  Lines       84899    85140   +241     
========================================
+ Hits        71824    72032   +208     
- Misses      13075    13108    +33

Files with missing lines	Coverage Δ
src/Groups/pcgroup.jl	`75.34% <100.00%> (+4.61%)`	⬆️

... and 57 files with indirect coverage changes

fingolfin · 2024-10-15T15:41:20Z

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.

jamesnohilly · 2024-10-16T11:31:58Z

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.

lgoettgens

some comments. Maybe more directed towards @fingolfin and @ThomasBreuer instead of @jamesnohilly

src/Groups/pcgroup.jl

test/Groups/pcgroup.jl

fingolfin · 2024-10-30T20:36:37Z

@jamesnohilly tests are still failing. are on onto that, or do you need assistance?

test/Groups/pcgroup.jl

src/Groups/pcgroup.jl

fingolfin · 2024-11-12T21:37:02Z

src/Groups/pcgroup.jl

+julia> gg = pc_group(c)
+Pc group of order 6
+
+julia> letters(gg[1]^5*gg[2]^-4)


This example is not quite obvious for a reader. Better to show the intermediate result, that should make the connection to the output of letters "obvious" :-)

Suggested change

julia> letters(gg[1]^5*gg[2]^-4)

julia> x = gg[1]^5*gg[2]^-4

f1*f2^2

julia> letters(x)

Good idea! I have updated the example in both syllables and letters to make this output clearer.

fingolfin · 2024-11-12T21:38:24Z

src/Groups/pcgroup.jl

+julia> gg = pc_group(c)
+Pc group of order 6
+
+julia> syllables(gg[1]^5*gg[2]^-4)


Suggested change

julia> syllables(gg[1]^5*gg[2]^-4)

julia> x = gg[1]^5*gg[2]^-4

f1*f2^2

julia> syllables(x)

fingolfin · 2024-11-12T21:40:30Z

src/Groups/pcgroup.jl

+julia> c = collector(2, Int);
+
+julia> Oscar.set_relative_orders!(c, [2, 3])
+
+julia> Oscar.set_conjugate!(c, 2, 1, [2 => 2])
+
+julia> gg = pc_group(c)


The interface for construction collectors is quite advanced and currently not even fully documented, so using it for an example is maybe not optimal. I suggest to use the small groups library to get a suitable example group instead, i.e. here we can just do this:

Suggested change

julia> c = collector(2, Int);

julia> Oscar.set_relative_orders!(c, [2, 3])

julia> Oscar.set_conjugate!(c, 2, 1, [2 => 2])

julia> gg = pc_group(c)

julia> gg = small_group(6,1)

and then I think the rest of the example stays unchanged.

Same remark applies below.

Perfect, no worries! I have now updated the examples with this replacement.

fingolfin · 2024-11-12T21:41:03Z

src/Groups/pcgroup.jl

+end
+
+# Convert syllables in canonical form into group element
+#Thomas


Suggested change

#Thomas

fingolfin · 2024-11-12T21:42:04Z

src/Groups/pcgroup.jl

+    syllables(g::Union{PcGroupElem, SubPcGroupElem})
+
+Return the syllables of `g` as a list of pairs of integers, each entry corresponding to
+a group generator and its exponent.


Maybe also mention and/or demonstrate in the example how to go back? My understanding is that gg(syllables(x)) == x, correct?

Yes that's correct! I have added a small demonstration at the bottom of the example in the recent commit.

fingolfin · 2024-11-12T21:43:37Z

src/Groups/pcgroup.jl

+
+# Convert syllables in canonical form into group element
+#Thomas
+function (G::PcGroup)(sylls::Vector{Pair{Int64, ZZRingElem}}; check::Bool=true)


Can you also add a similar constructor which takes an exponent vector, i.e., an inverse to letters?

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

function (G::FPGroup)(extrep::AbstractVector{T}) where T <: IntegerUnion

, which expects a flattened list of syllable pairs instead.

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.

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.

ugh, OK. perhaps we should kill that (is it documented?) first then.

maybe @ThomasBreuer can look into that?

fingolfin · 2024-11-13T08:48:00Z

src/Groups/pcgroup.jl

+    letters(g::Union{PcGroupElem, SubPcGroupElem})
+
+Return the letters of `g` as a list of integers, each entry corresponding to
+a group generator.


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).

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.

fingolfin · 2024-11-13T08:48:19Z

src/Groups/pcgroup.jl

+end
+
+# Convert syllables in canonical form into exponent vector
+#Thomas


Suggested change

#Thomas

lgoettgens · 2024-11-15T15:09:57Z

src/Groups/pcgroup.jl

+
+julia> gg(s) == x
+true
+"""


you are missing the three backticks here to end the doctest

good catch! I have fixed it now, thanks!

lgoettgens · 2024-11-15T16:37:07Z

This PR now unfortunately has conflicts with #4157.

Add syllable functions to pcgroup

e9a4ac4

Moved from the mjrodgers-OW_GModules branch.

joschmitt added the topic: groups label Oct 14, 2024

jamesnohilly added 2 commits October 17, 2024 13:35

Add letters function for PcGroupElem

b910923

Add testset for PcGroup letters function

d362f67

lgoettgens reviewed Oct 17, 2024

View reviewed changes

src/Groups/pcgroup.jl Outdated Show resolved Hide resolved

src/Groups/pcgroup.jl Outdated Show resolved Hide resolved

src/Groups/pcgroup.jl Outdated Show resolved Hide resolved

src/Groups/pcgroup.jl Show resolved Hide resolved

jamesnohilly added 3 commits October 17, 2024 16:31

Add support for SubPcGroupElem in letters

27c24e7

Cleanup code

89b8fe6

Rename pcgroup exponent_vector function

94cd03e

fingolfin reviewed Oct 22, 2024

View reviewed changes

test/Groups/pcgroup.jl Outdated Show resolved Hide resolved

jamesnohilly added 3 commits October 30, 2024 22:30

Fix test cases for finite pcgroup

416fd8b

Add check for syllables in canonical form

c350414

Add testset for canonical form check

b552be6

jamesnohilly marked this pull request as ready for review October 31, 2024 12:09

lgoettgens reviewed Oct 31, 2024

View reviewed changes

test/Groups/pcgroup.jl Outdated Show resolved Hide resolved

src/Groups/pcgroup.jl Outdated Show resolved Hide resolved

src/Groups/pcgroup.jl Outdated Show resolved Hide resolved

jamesnohilly added 2 commits November 1, 2024 13:50

Refactor based on PR comments

11bdb25

Document pcgroup syllable function

1b80584

fingolfin reviewed Nov 12, 2024

View reviewed changes

src/Groups/pcgroup.jl Outdated

end

# Convert syllables in canonical form into group element

#Thomas

Copy link

Member

fingolfin Nov 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

#Thomas

fingolfin reviewed Nov 12, 2024

View reviewed changes

fingolfin reviewed Nov 13, 2024

View reviewed changes

src/Groups/pcgroup.jl Outdated

end

# Convert syllables in canonical form into exponent vector

#Thomas

Copy link

Member

fingolfin Nov 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

#Thomas

Improve letters and syllables documentation

2d0bfb8

lgoettgens reviewed Nov 15, 2024

View reviewed changes

Fix syllables doctest

e7f4da0

benlorenz mentioned this pull request Nov 15, 2024

add two missing doctest end markers #4321

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `letters` function for PcGroupElem #4202

Add `letters` function for PcGroupElem #4202

jamesnohilly commented Oct 14, 2024 •

edited

Loading

codecov bot commented Oct 14, 2024 •

edited

Loading

fingolfin commented Oct 15, 2024

jamesnohilly commented Oct 16, 2024

lgoettgens left a comment

fingolfin commented Oct 30, 2024

fingolfin Nov 12, 2024

jamesnohilly Nov 15, 2024

fingolfin Nov 12, 2024

fingolfin Nov 12, 2024

jamesnohilly Nov 15, 2024

fingolfin Nov 12, 2024

fingolfin Nov 12, 2024

jamesnohilly Nov 15, 2024

fingolfin Nov 12, 2024

lgoettgens Nov 13, 2024

fingolfin Nov 13, 2024

lgoettgens Nov 13, 2024

lgoettgens Nov 13, 2024

fingolfin Nov 13, 2024 •

edited by lgoettgens

Loading

jamesnohilly Nov 15, 2024

fingolfin Nov 13, 2024

lgoettgens Nov 15, 2024

jamesnohilly Nov 15, 2024

lgoettgens commented Nov 15, 2024

-julia> letters(gg[1]^5*gg[2]^-4)
+julia> x = gg[1]^5*gg[2]^-4
+f1*f2^2
+julia> letters(x)

Add letters function for PcGroupElem #4202

Are you sure you want to change the base?

Add letters function for PcGroupElem #4202

Conversation

jamesnohilly commented Oct 14, 2024 • edited Loading

codecov bot commented Oct 14, 2024 • edited Loading

Codecov Report

fingolfin commented Oct 15, 2024

jamesnohilly commented Oct 16, 2024

lgoettgens left a comment

Choose a reason for hiding this comment

fingolfin commented Oct 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fingolfin Nov 13, 2024 • edited by lgoettgens Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgoettgens commented Nov 15, 2024

Add `letters` function for PcGroupElem #4202

Add `letters` function for PcGroupElem #4202

jamesnohilly commented Oct 14, 2024 •

edited

Loading

codecov bot commented Oct 14, 2024 •

edited

Loading

fingolfin Nov 13, 2024 •

edited by lgoettgens

Loading