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

Fix error in MaximalAbelianQuotient applied to a subgroup of an FPGroup which could result in wrong results #5620

Merged
merged 3 commits into from
Feb 2, 2024

Conversation

hulpke
Copy link
Contributor

@hulpke hulpke commented Feb 2, 2024

Text for release notes

An error in evaluating the MaximalAbelianQuotient of a subgroup of an FPGroup was fixed. This error could have produced wrong results.

Do not try the orbit mapping test for cyclic subgroups (or subgroups with
many orbits). Thiscould be memory intensive, and is done reasonably by
backtrack.

This resolves gap-system#5564
Replace method for `MaximalAbelianQuotient` for a subgroup with an easier
version that rewrites the presentation and then abelianizes.

This fixes gap-system#5609
Only attempt refinements with PCore, Agemo, Omega, if steps are not already
elementary abelian. This can avoid expensive subcomputations.
@hulpke hulpke added the release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes label Feb 2, 2024
@fingolfin fingolfin added kind: bug Issues describing general bugs, and PRs fixing them kind: bug: wrong result Issues describing bugs that result in mathematically or otherwise wrong results, and PRs fixing them labels Feb 2, 2024
@fingolfin fingolfin changed the title Fixes for #5609 and #5564 Fix an error in evaluating the MaximalAbelianQuotient of a subgroup of an FPGroup which could result in wrong results Feb 2, 2024
@fingolfin fingolfin added release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes regression A bug that only occurs in the branch, not in a release labels Feb 2, 2024
@fingolfin fingolfin merged commit c4fff6e into gap-system:master Feb 2, 2024
22 checks passed

#InstallMethod(MaximalAbelianQuotient,
# "subgroups of fp. abelian rewriting", true, [IsSubgroupFpGroup], 0,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason for commenting this out rather than just deleting it? The function can be recovered from git if it's needed again.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was too slow...

@fingolfin fingolfin changed the title Fix an error in evaluating the MaximalAbelianQuotient of a subgroup of an FPGroup which could result in wrong results Fix error in MaximalAbelianQuotient applied to a subgroup of an FPGroup which could result in wrong results Feb 5, 2024
@fingolfin fingolfin removed the release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes label Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: bug: wrong result Issues describing bugs that result in mathematically or otherwise wrong results, and PRs fixing them kind: bug Issues describing general bugs, and PRs fixing them regression A bug that only occurs in the branch, not in a release release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes
Projects
None yet
3 participants