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 cardinality of special linear group #36881

Merged

Conversation

RuchitJagodara
Copy link
Contributor

@RuchitJagodara RuchitJagodara commented Dec 14, 2023

  • Fixed infinite recursion of the cardinality and order functions
  • Improved pycodestyle
  • Add more doctests to cover all cases

This patch fixes #36876 and fixes #35490. In this PR, I have implemented a method to fix the
answer given by cardinality function for finite special linear groups
(before it was going into infinite recursion). This also fixes the answer given by is_finite()
function for finite special linear groups.

📝 Checklist

  • The title is concise, informative, and self-explanatory.
  • 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.

⌛ Dependencies

None

@RuchitJagodara RuchitJagodara force-pushed the cardinality_fot_small_spelcial_groups branch from 9cb6027 to 7a28d2f Compare December 17, 2023 18:45
@RuchitJagodara RuchitJagodara changed the title Implement method to find correct cardinality of special linear group Fix cardinality of special linear group Dec 23, 2023
@RuchitJagodara
Copy link
Contributor Author

RuchitJagodara commented Dec 23, 2023

@dimpase, @jukkakohonen and @videlec can you please review this PR ?

@RuchitJagodara RuchitJagodara marked this pull request as draft December 25, 2023 09:10
@mantepse
Copy link
Collaborator

Have you located the reason for the infinite recursion?

@RuchitJagodara
Copy link
Contributor Author

Yes, the is_finite() function is calling this cardinality function, and it seems that it is calling the order function (Considering that other classes have their own order function). I didn't find any special function that calculates the cardinality of these special linear groups. So, maybe it is not implemented yet, so it is calling the order function of some class from which it is derived, but that function is calling back this cardinality function.

@dimpase
Copy link
Member

dimpase commented Dec 25, 2023

Orders of special linear groups over fields are known, you can just implement formulae.

Less sure for more general cases

@RuchitJagodara
Copy link
Contributor Author

RuchitJagodara commented Dec 27, 2023

Orders of special linear groups over fields are known, you can just implement formulae.

I checked and found that order function is implemented only for gap-based groups and there is only one function available for other groups which is going in infinite recursion. Here below is a example of a gap-based special linear group which is working perfect.

sage: F = FiniteField(3)
sage: SL(2,F).order()
24

But when it comes to other groups which are not gap-based then it is going in infinite recursion like below

sage: q = 7
....: FqT.<T> = GF(q)[]
....: N = T^2+1
....: FqTN = QuotientRing(FqT, N*FqT)
....: S = SL(2,FqTN)
....: S.order()
---------------------------------------------------------------------------
RecursionError                            Traceback (most recent call last)
...

This is happening because in below code it is first trying to make an object of LinearMatrixGroup_gap but if it fails then it is simply making an object of LinearMatrixGroup_generic and that class does not have properties of gap and because of that it is not able to calculate order/cardinality of the gorup.

try:
cmd = 'SL({0}, {1})'.format(degree, ring._gap_init_())
return LinearMatrixGroup_gap(degree, ring, True, name, ltx, cmd,
category=cat)
except ValueError:
pass
return LinearMatrixGroup_generic(degree, ring, True, name, ltx,
category=cat)

@mantepse
Copy link
Collaborator

But when it comes to other groups which are not gap-based then it is going in infinite recursion like below

sage: q = 7
....: FqT.<T> = GF(q)[]
....: N = T^2+1
....: FqTN = QuotientRing(FqT, N*FqT)
....: S = SL(2,FqTN)

Should we be able to iterate over such a group?

@dimpase
Copy link
Member

dimpase commented Dec 27, 2023

There should be a special .order() method for SL. For finite fields $\mathbb{F}_q$ one has

$|SL(n,q)|={\frac{1}{q-1}} \Pi_{k=0}^{n-1} (q^n-q^k).$

Here FqTN is isomorphic to $\mathbb{F}_{49}$, so just use the forrmula, with $n=2$, $q=49$.

@RuchitJagodara
Copy link
Contributor Author

I have implemented the order function for linear matrix groups. I discovered that GAP does not support linear matrix groups over the rational field (and some other fields), which was the main cause of the error. Consequently, I attempted to address this issue in GAP, but it proved to be very non-trivial. Therefore, I implemented the function in Sage. Additionally, I have another question: Why do we use functions from GAP when we can implement all of those functions from scratch in Sage as well? In this case, we would have the flexibility to construct functions according to our needs. I understand it may take considerable time, but we can proceed step by step. The same can be applied to Magma, which was also causing some errors, as can be found in the Discussion and in issue #36841.

@RuchitJagodara RuchitJagodara marked this pull request as ready for review January 29, 2024 14:35
@RuchitJagodara
Copy link
Contributor Author

@dimpase and @mantepse, can you please review this pr...

src/sage/groups/matrix_gps/linear.py Show resolved Hide resolved
src/sage/groups/matrix_gps/linear.py Outdated Show resolved Hide resolved
src/sage/groups/matrix_gps/linear.py Show resolved Hide resolved
src/sage/groups/matrix_gps/linear.py Outdated Show resolved Hide resolved
src/sage/groups/matrix_gps/linear.py Outdated Show resolved Hide resolved
@RuchitJagodara RuchitJagodara force-pushed the cardinality_fot_small_spelcial_groups branch from 91ec122 to baa3b50 Compare February 9, 2024 09:41
@RuchitJagodara
Copy link
Contributor Author

I have also included a documentation change in this pr only because it is a very small change. Is it okay ?

Copy link

github-actions bot commented Feb 9, 2024

Documentation preview for this PR (built with commit baa3b50; changes) is ready! 🎉

Copy link
Collaborator

@mantepse mantepse left a comment

Choose a reason for hiding this comment

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

Thank you!

q = self.base_ring().order()
ord = prod(q**n - q**i for i in range(n))
if self._special:
return ord / (q-1)
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be //.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Possibly yes, but it doesn't look serious to me. Since this ticket is about to be merged, please open a new one!

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes but also no, rational numbers behave weirdly, e.g. is_prime(7 / 1) is False. But I will open a new PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@grhkm21, have you made another PR for the changes you suggested? If not then, should I create a new PR for this?

@vbraun vbraun merged commit 6ad8584 into sagemath:develop Feb 13, 2024
23 checks passed
@mkoeppe mkoeppe added this to the sage-10.3 milestone Mar 7, 2024
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
vbraun pushed a commit to vbraun/sage that referenced this pull request May 24, 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cardinality fails in small special linear groups Infinite recursion in LinearMatrixGroup_generic
6 participants