Fix order computation for linear groups GL(n, R) and SL(n, R) #37980
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Amend: Fix trailing whitespace
|
Documentation preview for this PR (built with commit 0be9db8; changes) is ready! 🎉 |
S17A05
changed the title
tscrim
requested changes
May 14, 2024
There was a problem hiding this comment.
Mostly a bunch of little things. We do not want to return Python ints. We can take advantage of the closure in Python. It would be good to add a test verifying the result:
sage: G = GL(2, Integers(6))
sage: G.order() == len(list(G))
True
sage: G = SL(2, Integers(6))
sage: G.order() == len(list(G))
True
|
Not sure why the tests failed, but the error makes it seem like a CI issue. |
|
Indeed, something seems borked with it. |
vbraun
pushed a commit
to vbraun/sage
that referenced
this pull request
May 18, 2024
…d SL(n, R)
sagemath#36881 introduced some wrong results for orders of linear groups since
it did not check whether the base ring is a field. We add the necessary
checks as well as formulas for some further cases, namely $\mathbb{Z}$
and $\mathbb{Z}/q\mathbb{Z}.$ Finally, we add a `NotImplementedError`
for the cases that are not supported.
The formula for the general linear group over $\mathbb{Z}/q\mathbb{Z}$
is taken from https://math.stackexchange.com/a/2044571, and for the
order of the special linear group we use the group isomorphism induced
by the determinant.
Fixes sagemath#37934.
URL: sagemath#37980
Reported by: Sebastian A. Spindler
Reviewer(s): Travis Scrimshaw
vbraun
pushed a commit
to vbraun/sage
that referenced
this pull request
May 18, 2024
…d SL(n, R)
sagemath#36881 introduced some wrong results for orders of linear groups since
it did not check whether the base ring is a field. We add the necessary
checks as well as formulas for some further cases, namely $\mathbb{Z}$
and $\mathbb{Z}/q\mathbb{Z}.$ Finally, we add a `NotImplementedError`
for the cases that are not supported.
The formula for the general linear group over $\mathbb{Z}/q\mathbb{Z}$
is taken from https://math.stackexchange.com/a/2044571, and for the
order of the special linear group we use the group isomorphism induced
by the determinant.
Fixes sagemath#37934.
URL: sagemath#37980
Reported by: Sebastian A. Spindler
Reviewer(s): Travis Scrimshaw
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
#36881 introduced some wrong results for orders of linear groups since it did not check whether the base ring is a field. We add the necessary checks as well as formulas for some further cases, namely$\mathbb{Z}$ and $\mathbb{Z}/q\mathbb{Z}.$ Finally, we add a
NotImplementedErrorfor the cases that are not supported.The formula for the general linear group over$\mathbb{Z}/q\mathbb{Z}$ is taken from https://math.stackexchange.com/a/2044571, and for the order of the special linear group we use the group isomorphism induced by the determinant.
Fixes #37934.