8000 Fix a mismatch between `AbelianGroup` class `permutation_group` method documentation and implementation by reiniscirpons · Pull Request #40267 · sagemath/sage · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Fix a mismatch between AbelianGroup class permutation_group method documentation and implementation #40267

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

Fix a mismatch between AbelianGroup class permutation_group method documentation and implementation #40267

wants to merge 3 commits into from

Conversation

reiniscirpons
Copy link

This PR fixes issue #39890 . Namely, the docs of permutation_group for AbelianGroup say that:

If the invariants are $q_1, \ldots, q_n$ then the generators of the permutation will be of order $q_1, \ldots, q_n$, respectively.

However the previous implementation of the method constructed the permutation group using the GAP IsomorphismPermGroup function, which makes no guarantees about the generators of the permutation group it finds (perhaps this has changed between the initial implementation and now). I've fixed the issue by explicitly computing the disjoint permutations corresponding to each generator.

📝 Checklist

  • The title is concise and informative.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation and checked the documentation preview.

⌛ Dependencies

No PR dependencies

@reiniscirpons reiniscirpons changed the title Fix mismatch AbelianGroup class permutation_group method documentation and implementation Fix a mismatch between AbelianGroup class permutation_group method documentation and implementation Jun 18, 2025
@user202729
Copy link
Contributor

I think it's preferred (and potentially faster?) to just send the generators to gap through the isomorphism itself?

Benchmark needed.

@reiniscirpons
Copy link
Author

I think it's preferred (and potentially faster?) to just send the generators to gap through the isomorphism itself?

Benchmark needed.

I hadn't thought of that. It could be quicker yes!

I'm new to sage development, how would I go about benchmarking different implementations of the function? Is the preferred method to just use the timeit function or the sage_timeit function? I understand one calls the other from the docs, but its unclear which should be preferred for this type of bench.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
s: needs review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@reiniscirpons @user202729 @fchapoton
0