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.

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

Recent changes cause test suites of groupoids, semigroups, xmod to fail #5318

Closed
fingolfin opened this issue Jan 12, 2023 · 14 comments
Closed
Labels
regression A bug that only occurs in the branch, not in a release topic: packages issues or PRs related to package handling, or specific to a package (for packages w/o issue tracker)

Comments

@fingolfin
Copy link
Member

See this report for links to the failing CI runs.

It seems to have started between January 9-11, but unfortunately during that day GitHub was experiencing trouble and a result many CI jobs randomly failed everywhere, hence https://github.com/gap-system/PackageDistro/blob/data/reports/master/2023-01-11-03:00:40-4a91c2d8/report.md shows other unrelated failures (mostly due to failed downloads).

I have not yet made attempts to figure out if the changes are legit and harmless, and just require adjusting the tests to make them more robust (e.g. avoid depending on some GAP computation choosing specific permutations to generate some subgroup); or whether there are genuine mathematical issues (and in that case, whether the new code is buggy, or the code in the package, or something else). But at least for semigroups, the result of a InverseSemigroup computation changed from <partial perm group of rank 8 with 1 generator> to something with rank 4, which sounds like a real problem to me.

I don't know for sure which PR started it, but my suspicions are on PR #5298 by @hulpke or PR #5275 by @james-d-mitchell. (To be clear, I am not implying these PRs are doing anything wrong, just that one of them or perhaps even the combination, likely started the failure of those package CI tests.)

CC @james-d-mitchell @cdwensley

@fingolfin fingolfin added regression A bug that only occurs in the branch, not in a release topic: packages issues or PRs related to package handling, or specific to a package (for packages w/o issue tracker) labels Jan 12, 2023
@james-d-mitchell
Copy link
Contributor

A quick git bisect reveals that (for semigroups at least), the commit which introduced the breaking change is:

8cbcc1f is the first bad commit
commit 8cbcc1f
Author: Alexander Hulpke [email protected]
Date: Mon Dec 26 11:31:40 2022 -0700

ENHANCE: Better permrep for pc groups

This is relevant when forming factor groups as subdirect products.

@fingolfin
Copy link
Member Author

Thanks for that, @james-d-mitchell. I assume the failures in the semigroups test suite suggest a real problem (not just "different output due to randomness"), correct? I wonder if we can isolate an example of a problematic input for IsomorphismPermGroup

@james-d-mitchell
Copy link
Contributor

Thanks @fingolfin, I haven't checked in detail, this isn't a "different output due to randomness issue" as far as I can tell.
It looks like the image of IsomorphismPermGroup acts on fewer points than previously, I will investigate further just now.

@hulpke
Copy link
Contributor

hulpke commented Jan 12, 2023

This probably means that code (or an example) makes an implicit assumption of a permutation representation chosen (regular? transitive?) that is not true any longer. But if you can point me to a pc group for which the perm rep computed now is false, I will fix ASAP.

@fingolfin
Copy link
Member Author

(If such an input exists -- for xmod, so far everything looks fine to me -- different results, but they seem equally valid)

@james-d-mitchell
Copy link
Contributor

Here is an example where the behaviour is different before and after 8cbcc1f.

Before:

gap> G := Range(IsomorphismPermGroup(SmallGroup(12, 1)));
Group([ (1,2,3,5)(4,10,7,12)(6,11,9,8), (1,3)(2,5)(4,7)(6,9)(8,11)(10,12), (1,4,8)(2,6,10)(3,7,11)(5,9,12) ])
gap> IdSmallGroup(G);
[ 12, 1 ]

After:

gap> G := Range(IsomorphismPermGroup(SmallGroup(12, 1)));
Group([ (1,2,3,4)(6,7), (1,3)(2,4), (5,6,7) ])
gap> IdSmallGroup(G);
[ 12, 1 ]

So the return value of IsomorphismPermGroup seems to be valid, just different.

@hulpke
Copy link
Contributor

hulpke commented Jan 12, 2023

Yes, IsomorphismPermGroup for Pc groups will in most cases return a smaller, and often intransitive representation. (It always used to be the regular representation, which can cause serious inefficiencies.)

@cdwensley
Copy link
Contributor

I am happy to adjust the output in examples in XMod and Groupoids, but not just yet. I am in the process of replacing my 2009 iMac, which is no longer fit for purpose, and have removed my entire GAP installation.

@james-d-mitchell
Copy link
Contributor

I'm also willing to adjust the output for Semigroups, and have opened an issue in Semigroups so we don't forget.

@fingolfin
Copy link
Member Author

@james-d-mitchell so the changes are not mathematical problems? Good!

One way to get the tests to produce identical results across GAP versions might be to use RegularActionHomomorphism instead of IsomorphismPermGroup. Of course this mostly makes sense for examples were you want stable output; for code where one really wants a permutation groups that is "as efficient as possible" it usually a good thing to have the smaller degree (which is of course why @hulpke implemented this nice optimization).

@cdwensley
Copy link
Contributor

In both XMod and Groupoids have now added functions which call RegularActionHomomorphism and used these functions in tests in place of those which use IsomorphismPermGroup.

@fingolfin
Copy link
Member Author

@cdwensley thank you, looking forward to new releases with these fixes in them then!

@cdwensley
Copy link
Contributor

Groupoids 1.73 and XMod 2.91 now released with these fixes.

@fingolfin
Copy link
Member Author

All seems to be working now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
regression A bug that only occurs in the branch, not in a release topic: packages issues or PRs related to package handling, or specific to a package (for packages w/o issue tracker)
Projects
None yet
Development

No branches or pull requests

4 participants