Skip to content

Conversation

fingolfin
Copy link
Member

@fingolfin fingolfin commented Jan 16, 2025

This used to run into a coset enumeration.

Resolves #3898
Resolves #5910

@fingolfin fingolfin added kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements topic: library release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes labels Jan 16, 2025
@fingolfin fingolfin force-pushed the mh/fix-Comm-for-autos branch 2 times, most recently from de18fb0 to b8b74ab Compare January 17, 2025 11:21
Copy link
Contributor

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

Yes, this is a solution.

Besides that (something for another pull request), I think that it would be desirable that special (partial) methods for composition mappings are preferred to other methods.
In the current example, the IsInjective method for composition mappings is what one expects to be checked, but it has a lower rank than the method for group homomorphisms.

Copy link
Contributor

@hulpke hulpke left a comment

Choose a reason for hiding this comment

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

Yes, this fixes the problem at hand. But doesn't it push the coset enumeration to the bit when the homomorphism gets evaluated on elements?

@fingolfin
Copy link
Member Author

Evaluation on elements seems to work fine:

gap> hom(x1);
(x2*x3^-1)^4*x1*(x3^-1*x2)^4
gap> hom(x2);
(x2*x3^-1)^8*x2

@fingolfin
Copy link
Member Author

That said, this does indeed "only" solve part of the issue; one may argue it "papers over the real issue" -- I guess that's why nobody applied this fix despite it being suggested 5 years ago... Since nobody fixed the "real" issue in 5 years, I thought it is a bit silly to refuse to apply the immediate fix. We can still track the "real" issue afterwards if we want to, and perhaps one day someone will address it :-)

@fingolfin
Copy link
Member Author

I've revised this now to instead uprank IsInjective and other methods specific for IsCompositionMappingRep. I've also extended and refined the test case a bit.

This used to run into a coset enumeration.

We increase the rank of certain methods for composite maps
to resolve the issue and possibly others.
@fingolfin fingolfin force-pushed the mh/fix-Comm-for-autos branch from a27f569 to 4f463f3 Compare January 20, 2025 12:59
@fingolfin
Copy link
Member Author

@ThomasBreuer does this look reasonable to you?

@ThomasBreuer
Copy link
Contributor

Yes, this looks reasonable.

I think that in general, an available method that is installed for composition mappings should be called first for a composition mapping, because delegating to the maps which define the composition is the most natural strategy --other methods have less information. In particular, in situations where the defining maps (or perhaps all of them except one) store already that they are bijective, the question ends up in the right place.

If this change should cause problems in certain situations then I expect that a solution will be not to construct a composition mapping but something better in that situation.

The same strategy should be used also for computing images and preimages. As far as I see, this is not a problem because we have essentially the methods for the different representations of mappings, and these methods are not competing for applicability.

@fingolfin fingolfin merged commit c4a13f4 into master Jan 21, 2025
37 checks passed
@fingolfin fingolfin deleted the mh/fix-Comm-for-autos branch January 21, 2025 21:42
fingolfin added a commit that referenced this pull request Jan 24, 2025
This used to run into a coset enumeration.

We increase the rank of certain methods for composite maps
to resolve the issue and possibly others.
@fingolfin
Copy link
Member Author

Backported to stable-4.14 in 6611874

@fingolfin fingolfin added backport-to-4.14-DONE-unused Renamed so release scripts ignore them; we skipped 4.14.1 and did not use backports and removed backport-to-4.14 labels Jan 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-to-4.14-DONE-unused Renamed so release scripts ignore them; we skipped 4.14.1 and did not use backports kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes topic: library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve IsInjective (and other?) methods for composite maps Computation of Comm() of two free group automorphisms invokes coset enumeration
3 participants