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

[Merged by Bors] - feat: Cardinal of GLn with coefficients in a finite field #14095

Closed
wants to merge 17 commits into from

Conversation

ThomasLanard
Copy link
Collaborator


We add the cardinal of the general linear group GLn with coefficients in a finite field.

Open in Gitpod

@ThomasLanard ThomasLanard added the new-contributor This PR was made by a contributor with at most 5 merged PRs. Welcome to the community! label Jun 24, 2024
Copy link

github-actions bot commented Jun 24, 2024

PR summary 5ad36cbcc7

Import changes for modified files

No significant changes to the import graph

Import changes for all files
Files Import difference
Mathlib.LinearAlgebra.Matrix.GeneralLinearGroup.Card 1207

Declarations diff

+ card_GL_field
+ card_linearIndependent
+ equiv_GL_linearindependent
+ equiv_linearIndependent

You can run this locally as follows
## summary with just the declaration names:
./scripts/no_lost_declarations.sh short <optional_commit>

## more verbose report:
./scripts/no_lost_declarations.sh <optional_commit>

@CBirkbeck CBirkbeck self-requested a review June 24, 2024 15:26
Copy link
Collaborator

@CBirkbeck CBirkbeck left a comment

Choose a reason for hiding this comment

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

I've left some comments on the code, it looks pretty good to me, only some minor fixes/golfing.

@ThomasLanard
Copy link
Collaborator Author

Thank you! I've made the corrections that were suggested.

@grunweg
Copy link
Collaborator

grunweg commented Jun 25, 2024

Coming here for issue triage: it seems you're looking for review on this PR; I have labelled it awaiting-review accordingly.
Also note that this PR cannot be merged while it is in draft mode. If you consider it ready for review, would you like to un-draft it? (Draft PRs also do not show up in the review queue.)

@ThomasLanard ThomasLanard marked this pull request as ready for review June 25, 2024 14:59
@riccardobrasca riccardobrasca self-assigned this Jun 28, 2024
@riccardobrasca
Copy link
Member

Can you please mark "resolved" the comments that you took into account?

There is also an error related to imports (this should be very easy to resolve). Thanks!

Copy link
Member

@riccardobrasca riccardobrasca left a comment

Choose a reason for hiding this comment

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

Thinking a little bit more about it, maybe you can use Nat.card and completely avoid various Fintype instances. It seems to me the best way of doing stuff. Note that you may need the instances in proofs (to make the link between Nat.card and Fintype.card, that has a better API), but there is no need to create global instances.

@ThomasLanard
Copy link
Collaborator Author

Thinking a little bit more about it, maybe you can use Nat.card and completely avoid various Fintype instances. It seems to me the best way of doing stuff. Note that you may need the instances in proofs (to make the link between Nat.card and Fintype.card, that has a better API), but there is no need to create global instances.

I changed the Fintype.card into Nat.card and suppressed all the instances (hence, also the modification in the file about Clifford algebra).

@riccardobrasca
Copy link
Member

I've investigated what's happening here, and the problem is that you are changing the structure of the import graph, and this is making things too slow in other files that import GeneralLinearGroup. My suggestion is to create a new folder GeneralLinearGroup, with files Basic (corresponding to what already is in master) and a file Card, where you can put your result, importing whatever you need. We have a command find_home to see where we can put files without changing the imports and it seems impossible to do so for your card_LinearInependent_subtype.

@riccardobrasca
Copy link
Member

Even better, we may have three files: Defs, Basic and Card, this is what we usually do nowadays. Maybe a preliminary PR doing the split is the way to go.

@ThomasLanard
Copy link
Collaborator Author

After thinking about it, I found it a bit weird to split the GeneralLinearGroup.lean file into a folder. For instance, there is also a SpecialLinearGroup.lean file and not a folder. Thus instead I added a Card.lean file into the folder Matrix which could contain different statements about cardinals of subsets of matrices. For instance, I added a simple result about the cardinal of the set of matrices over a finite field.

I moved equiv_linearIndependent to Mathlib.LinearAlgebra.LinearIndependent. But I don’t know where to put card_linearInependent.

@riccardobrasca
Copy link
Member

What I am proposing is to split the existing file GeneraliLinearGroup into two files GeneraliLinearGroup.Defs and ``GeneraliLinearGroup.Basic, and then add a file GeneraliLinearGroup.Card` containing your result. It's better to do the splitting in another PR (this kind of PRs are usually reviewed very quickly since nothing has really changed), and then updating this one. I know it looks a slow process, but mathlib is a huge library, and it is better to waste some time but doing things properly.

I can do it you want.

mathlib-bors bot pushed a commit that referenced this pull request Jul 5, 2024
Split of `LinearAlgebra/Matrix/GeneralLinearGroup.lean` into `LinearAlgebra/Matrix/GeneralLinearGroup/Defs.lean` and `LinearAlgebra/Matrix/GeneralLinearGroup/Basic.lean`. This is motivated by PR #14095.
@riccardobrasca
Copy link
Member

Now the file should go in the GeneralLinearGroup folder.

Copy link
Member

@riccardobrasca riccardobrasca left a comment

Choose a reason for hiding this comment

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

Thanks!

bors d+

@mathlib-bors
Copy link
Contributor

mathlib-bors bot commented Jul 5, 2024

✌️ ThomasLanard can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

@CBirkbeck
Copy link
Collaborator

Thanks for doing all of this!

@riccardobrasca
Copy link
Member

Interesting that we don't need commutativity for the formula to hold (I know commutativity is secretly there, but we don't need to know it).

@ThomasLanard
Copy link
Collaborator Author

ThomasLanard commented Jul 5, 2024

Interesting that we don't need commutativity for the formula to hold (I know commutativity is secretly there, but we don't need to know it).

I have imported Mathlib.RingTheory.LittleWedderburn to have commutativity! It is the only reason for this import.

@riccardobrasca
Copy link
Member

Interesting that we don't need commutativity for the formula to hold (I know commutativity is secretly there, but we don't need to know it).

I have imported Mathlib.RingTheory.LittleWedderburn to have commutativity! It is the only reason for this import.

Oh, I didn't notice it. I suggest to not import it and prove it for Field then.

@ThomasLanard
Copy link
Collaborator Author

Thank you!

bors r+

mathlib-bors bot pushed a commit that referenced this pull request Jul 5, 2024
@mathlib-bors
Copy link
Contributor

mathlib-bors bot commented Jul 5, 2024

Pull request successfully merged into master.

Build succeeded:

@mathlib-bors mathlib-bors bot changed the title feat: Cardinal of GLn with coefficients in a finite field [Merged by Bors] - feat: Cardinal of GLn with coefficients in a finite field Jul 5, 2024
@mathlib-bors mathlib-bors bot closed this Jul 5, 2024
@mathlib-bors mathlib-bors bot deleted the TL_card_GL_Fq branch July 5, 2024 15:27
@adomani adomani mentioned this pull request Aug 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
delegated new-contributor This PR was made by a contributor with at most 5 merged PRs. Welcome to the community!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants