Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

rename and change some *_reps functions #3291

Merged
merged 16 commits into from
Feb 16, 2024
Merged

rename and change some *_reps functions #3291

merged 16 commits into from
Feb 16, 2024

Conversation

ThomasBreuer
Copy link
Member

This is the non-controversial and non-breaking part of #3265:

Provide complement_classes instead of complement_class_reps,
hall_subgroups instead of hall_subgroup_reps,
low_index_subgroups instead of low_index_subgroups_reps.
Each of the new functions returns a vector of subgroup classes.

Also move the outdated hall_subgroup to deprecations.jl.

@codecov Codecov
Copy link

codecov bot commented Jan 31, 2024 •
edited
Loading

Codecov Report

Merging #3291 (aee2e7f) into master (070219e) will increase coverage by 0.00%.
The diff coverage is 78.68%.

Additional details and impacted files 
@@           Coverage Diff           @@
##           master    #3291   +/-   ##
=======================================
  Coverage   81.60%   81.60%           
=======================================
  Files         546      546           
  Lines       73504    73495    -9     
=======================================
- Hits        59980    59973    -7     
+ Misses      13524    13522    -2
Files Coverage Δ
src/Groups/GAPGroups.jl 92.88% <100.00%> (+0.97%) ⬆️
src/Groups/gsets.jl 88.72% <ø> (ø)
src/NumberTheory/GaloisGrp/Group.jl 66.66% <100.00%> (ø)
src/Groups/sub.jl 94.08% <0.00%> (-0.11%) ⬇️
src/NumberTheory/GaloisGrp/GaloisGrp.jl 74.14% <75.00%> (ø)
src/deprecations.jl 0.00% <0.00%> (ø)
src/Groups/GrpAb.jl 92.71% <73.07%> (+0.25%) ⬆️

... and 1 file with indirect coverage changes

@fingolfin
Copy link
Member

But if we do this then these functions behave inconsistent with maximal_subgroups and subgroups. So it is not clear to me that this is a definite "win" -- if we decide not to change those two, then I don't think we should make the changes here?

I'll discuss this some more with @fieker later.

@ThomasBreuer
Copy link
Member Author

@fingolfin Thanks for your comment.
From my point of view, a breaking change for maximal_subgroups* is not a problem.
For subgroups, we could proceed as proposed in #3265 (return conjugacy classes of subgroups) AND use a different name for the subgroups functionality that comes from Hecke.jl.

@ThomasBreuer
Copy link
Member Author

After the discussion of #3265, what about the names complement_classes, hall_subgroup_classes, low_index_subgroup_classes, maximal_subgroup_classes, and subgroup_classes?
Then the names are self-explanatory (I think), all changes are non-breaking, and also Hecke.jl's subgroups can be kept as is.

(If we take this solution then only subgroups/subgroup_reps/conjugacy_classes_subgroups needs further code changes, the rest is just string replacement.)

@fingolfin
Copy link
Member

That would be fine by me.

As a variation (possibly for a future PR, subgroups and friends could return an iterator instead of a vector.... eg subgroups (G::GAPGroups) = Iterators.flatten(subgroup_classes(G))

fingolfin
fingolfin reviewed Feb 2, 2024
View reviewed changes
src/Groups/GAPGroups.jl
```
"""
function complement_class_reps(G::T, N::T) where T <: GAPGroup
return _as_subgroups(G, GAP.Globals.ComplementClassesRepresentatives(G.X, N.X))
function complement_classes(G::T, N::T) where T <: GAPGroup
Copy link
Member

Choose a reason for hiding this comment

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

This is one of those signatures that needs to be relaxed when we introduce SubPcGroupElem, I guess...

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, and here we have also an example which is subtle in the sense that the result may be empty.
In this case, its type is that of an empty array of conjugacy classes of groups, where the type of each conjugacy class is parametrized by the types of the acting group and the representative. In the case of pc groups, the representatives will be of type SubPcGroup, and the acting group can be either a PcGroup or a SubPcGroup.

fingolfin
fingolfin reviewed Feb 14, 2024
View reviewed changes
src/Groups/GAPGroups.jl Outdated Show resolved Hide resolved
fingolfin
fingolfin reviewed Feb 14, 2024
View reviewed changes
src/Groups/GAPGroups.jl Outdated Show resolved Hide resolved
fingolfin
fingolfin reviewed Feb 14, 2024
View reviewed changes
src/Groups/GAPGroups.jl Outdated Show resolved Hide resolved
src/Groups/GAPGroups.jl Outdated Show resolved Hide resolved
src/Groups/GAPGroups.jl Outdated Show resolved Hide resolved
src/Groups/GAPGroups.jl Outdated Show resolved Hide resolved
src/Groups/GAPGroups.jl Outdated Show resolved Hide resolved
src/Groups/GAPGroups.jl Outdated Show resolved Hide resolved
src/Groups/sub.jl Show resolved Hide resolved
src/NumberTheory/GaloisGrp/GaloisGrp.jl Outdated Show resolved Hide resolved
src/NumberTheory/GaloisGrp/Group.jl Outdated Show resolved Hide resolved
@fingolfin fingolfin added the backport 1.0.x Should be backported to the release 1.0 branch label Feb 15, 2024
@fingolfin
Copy link
Member

Added "backport to 1.0" label as I need this for the book.

@ThomasBreuer
replace hall_subgroup_reps by hall_subgroups
03d9a59
@ThomasBreuer
replace lowindex_subgroups_reps by low_index_subgroups
165b874
@ThomasBreuer
replace complement_class_reps by complement_classes
0959035
@ThomasBreuer
replace maximal_subgroup_reps by maximal_subgroups
ab1905b 
- deprecate `conjugacy_classes_maximal_subgroups`, `maximal_subgroup_reps`
- let `maximal_subgroups` return subgroup classes
- adjust calls of these functions
@ThomasBreuer
adjust tests involving maximal_subgroups
23409fc
@ThomasBreuer
rename conjugacy_classes_subgroups to subgroup_classes
ab524ed 
and deprecate `subgroup_reps` and `subgroups` (the latter only for `GAPGroup`s)
@ThomasBreuer
rename *_subgroups to *_subgroup_classes
840f656 
for `*` one of `hall`, `maximal`, `low_index`
@ThomasBreuer
do not use deprecated functions in tests
9ad6636
@ThomasBreuer
adjust one more test using a deprecated function
48beaf9
@ThomasBreuer
adjust some doctest outputs again
0e46a7f
@ThomasBreuer
address comments
a48e8e5
@ThomasBreuer
add missing doc. for low_index_subgroup_classes
3b76df2 
Why did I not get an error in the local build?
@ThomasBreuer
fixes
ed9d640
@ThomasBreuer
address comments
4a02d18
@benlorenz benlorenz mentioned this pull request Feb 16, 2024
Merged
33 tasks
fingolfin
fingolfin approved these changes Feb 16, 2024
View reviewed changes
test/Groups/subgroups_and_cosets.jl Outdated Show resolved Hide resolved
test/Groups/subgroups_and_cosets.jl Outdated Show resolved Hide resolved
@fingolfin fingolfin enabled auto-merge (squash) February 16, 2024 13:14
@fingolfin fingolfin merged commit 2c41355 into oscar-system:master Feb 16, 2024
16 of 20 checks passed
benlorenz pushed a commit that referenced this pull request Feb 19, 2024
@ThomasBreuer @benlorenz
rename and change some *_reps functions (#3291)
3938ffe
@benlorenz benlorenz removed the backport 1.0.x Should be backported to the release 1.0 branch label Feb 19, 2024
@thofma thofma mentioned this pull request Feb 20, 2024
Merged
benlorenz added a commit that referenced this pull request Feb 23, 2024
@benlorenz
Backports for 1.0.0-rc1
1388932 
### Backported PRs

- [x] #3367
- [x] #3379 
- [x] #3369
- [x] #3291
- [x] #3325
- [x] #3350 
- [x] #3351
- [x] #3365 
- [x] #3366
- [x] #3382
- [x] #3373
- [x] #3341
- [x] #3346
- [x] #3381
- [x] #3385
- [x] #3387 
- [x] #3398 
- [x] #3399 
- [x] #3374 
- [x] #3406 
- [x] #2823
- [x] #3298
- [x] #3386 
- [x] #3412 
- [x] #3392 
- [x] #3415 
- [x] #3394
- [x] #3391
@ThomasBreuer ThomasBreuer deleted the TB_no_subgroup_reps branch February 28, 2024 13:39
@ThomasBreuer ThomasBreuer mentioned this pull request Feb 28, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thofma thofma thofma left review comments

@fingolfin fingolfin fingolfin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ThomasBreuer @fingolfin @thofma @benlorenz