Skip to content

Clean up some code that got obsolete by newer GAP#4464

Merged
lgoettgens merged 4 commits intooscar-system:masterfrom
lgoettgens:lg/GAP-cleanup
Jan 16, 2025
Merged

Clean up some code that got obsolete by newer GAP#4464
lgoettgens merged 4 commits intooscar-system:masterfrom
lgoettgens:lg/GAP-cleanup

Conversation

@lgoettgens
Copy link
Copy Markdown
Member

No description provided.

Comment thread gap/OscarInterface/PackageInfo.g Outdated
GAP := ">= 4.12",
GAP := ">= 4.14",
NeededOtherPackages := [
["JuliaInterface", ">=0.9"],
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

similar here

Copy link
Copy Markdown
Member

@ThomasBreuer ThomasBreuer left a comment

Choose a reason for hiding this comment

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

Thanks, looks good.

Comment thread test/Groups/gsets.jl Outdated
S = sylow_subgroup(GL, 2)[1]
for G in [GL, S]
# for k in 0:n # k = 0 is a problem in GAP 4.12.0
# for k in 0:n # k = 0 is a problem in GAP 4.14.0
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Meanwhile I think it is better to fix this on the Oscar side. I will create a pull request for that.

Comment thread gap/OscarInterface/PackageInfo.g Outdated
H = T(GAPWrap.ImagesSource(f))
# TODO: remove the next line once GAP 4.13.0 is available in Oscar
# TODO: remove the next line once GAP 4.13.0 is available in Oscar -> still broken in GAP 4.14.0
GAP.Globals.UseIsomorphismRelation(GapObj(G), GapObj(H))
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

at least for the case of PermGroups. After I remove this line as indicated in the comment:

julia> G = dihedral_group(6)
Pc group of order 6

julia> iso = isomorphism(PermGroup, G)
Group homomorphism
  from pc group of order 6
  to permutation group of degree 3 and order 6

julia> has_order(codomain(iso))
false

With the line still in, the last result is true instead.

This got caught by the doctest

to permutation group of degree 3 and order 6
that no longer printed the order of the codomain.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

When checking whether GAP now calls UseIsomorphismRelation, I tried a bigger group, and there is an early exit for very small groups.
The GAP code in question has several return statements, I have to think about a good solution.

@codecov
Copy link
Copy Markdown

codecov Bot commented Jan 14, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.39%. Comparing base (e993383) to head (c18c649).
Report is 49 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4464      +/-   ##
==========================================
- Coverage   84.41%   84.39%   -0.02%     
==========================================
  Files         668      668              
  Lines       88591    88603      +12     
==========================================
- Hits        74780    74779       -1     
- Misses      13811    13824      +13     
Files with missing lines Coverage Δ
...ogonalDiscriminants/src/OrthogonalDiscriminants.jl 100.00% <ø> (ø)
src/Groups/GAPGroups.jl 94.11% <100.00%> (ø)
src/Groups/homomorphisms.jl 91.70% <ø> (-0.03%) ⬇️

... and 4 files with indirect coverage changes

Copy link
Copy Markdown
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

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

Looks good to me

@lgoettgens lgoettgens enabled auto-merge (squash) January 15, 2025 14:35
@lgoettgens lgoettgens merged commit 24de938 into oscar-system:master Jan 16, 2025
@lgoettgens lgoettgens deleted the lg/GAP-cleanup branch January 16, 2025 16:39
@aaruni96 aaruni96 added the release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes label Jan 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

package: GAP release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants