Skip to content

Make some group tests more future-proof - #5283

Closed
ThomasBreuer wants to merge 6 commits into
oscar-system:masterfrom
ThomasBreuer:TB_update_tests_to_GAP
Closed

Make some group tests more future-proof#5283
ThomasBreuer wants to merge 6 commits into
oscar-system:masterfrom
ThomasBreuer:TB_update_tests_to_GAP

Conversation

@ThomasBreuer

@ThomasBreuer ThomasBreuer commented Sep 9, 2025

Copy link
Copy Markdown
Member

Make tests more robust against changed orderings on the GAP side, due to a new version of GAP (cf oscar-system/GAP.jl#1244 (comment))

And remove a special sub method for MatrixGroup, which was worse than the generic method.

@ThomasBreuer ThomasBreuer added topic: groups release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes labels Sep 9, 2025
julia> rays(c)
5-element SubObjectIterator{RayVector{QQFieldElem}}:
julia> sort(rays(c))
5-element Vector{RayVector{QQFieldElem}}:

@lgoettgens lgoettgens Sep 10, 2025

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.

Just sorting does not fix all the changes in the booktest that I posted in the list elsewhere

@lgoettgens lgoettgens changed the title adjust tests Make some group tests more future-proof Sep 10, 2025
@fingolfin fingolfin closed this Sep 11, 2025
@fingolfin fingolfin reopened this Sep 11, 2025
@lgoettgens

Copy link
Copy Markdown
Member

This should not be merged right now. It is more of a testground which changes could be made in Oscar to allow for the gap 4.15 update. But some of them will be discussed in oscar-system/GAP.jl#1244 or in gap and this PR should wait for the results.

true

julia> has_number_of_primitive_groups(5000)
julia> has_number_of_primitive_groups(10000)

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.

With gap-packages/primgrp#61 being available in primgrp 4.0.1, we don't need to do any of the changes concerning primitive groups (at least until #5293)

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.

However they also don't hurt us, right?

Comment thread src/Groups/matrices/MatGrp.jl Outdated
# Examples
```jldoctest
julia> tbl = character_table(alternating_group(4));
julia> tbl = character_table("A4");

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.

I am not quite sure that I understand the difference here. Does a string refer to some specific stored table in GAP, but when providing a group, it tries to recompute things (and thus may change the order of rows/cols)?

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.

exactly

Comment thread src/Groups/group_characters.jl Outdated
Comment on lines +1010 to +1011
sorttbl = GAP.Globals.CharacterTableWithSortedCharacters(sorttbl, pi)::GapObj
sorttbl = GAP.Globals.CharacterTableWithSortedCharacters(sorttbl)::GapObj

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.

these two lines look somehow redundant to me. But I just wanted to point this out and would otherwise trust you with this.

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.

indeed, odd. @ThomasBreuer ?

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.

Perhaps there is a reason for this double sorting, but then a comment should be added to explain that :-)

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.

The aim of the second call is to get the trivial character to the first position.
After sorting the Irr value lexicographically, it will be the last of the linear characters.
I will add a comment about this and rename the function.
I am not sure whether other testfiles will sooner or later also need such a hack, therefore I am not in favour of moving the function to the currently affected testfile.

Comment thread src/Groups/group_characters.jl Outdated
# for tests: sort a given character table heuristically,
# in order to achieve a more stable ordering of rows and columns
#
function _sort(tbl::GAPGroupCharacterTable)

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.

I would prefer a more descriptive name for this function, but I don't really have a great suggestion. My ideas (which I am not convinced by) are heuristic_sort, sort_for_stable_tests or something like that.

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.

If it only for tests and only used in a single file in test/Groups/, then perhaps we can just rename this to eg sort_for_stable_tests and move it to that test file?

With the next GAP version, the ordering of character tables changes.
The different ordering is a side-effect of a new GAP version.
The latest version of the PrimGrp package claims that
the primitive groups of degree up to 8191 are available.

In order to get a `false` result of `has_number_of_primitive_groups`,
degree `10000` will be safe for some time.
This reverts commit 8705889.
The commit did *not* fix some changed output;
and this changed output was not reproducible.
In order to get a `false` result of `has_number_of_primitive_groups`,
degree `10000` will be safe for some time.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

DO NOT MERGE release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes topic: groups

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants