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

Smith Normal Form #710

Merged
merged 30 commits into from
Apr 6, 2022
Merged

Smith Normal Form #710

merged 30 commits into from
Apr 6, 2022

Conversation

kangrongji
Copy link
Contributor

This PR contains the existence proof of Smith normal form for Integers, based on the divisibility result of #705. Hope this could pave the way towards a general theory for finitely presented abelian group, that used tons of times in algebraic topology. Maybe I would do some of them soon or later.

@ice1000
Copy link
Member

ice1000 commented Feb 9, 2022

Can you merge master and change your invocation of pos· to pos·pos?

@kangrongji
Copy link
Contributor Author

@ice1000 Done!

Copy link
Collaborator

@mortberg mortberg left a comment

Choose a reason for hiding this comment

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

Wow, a lot of very nice stuff! Just had a few small comments about minor code details.

Did you try to run the function to compute the SNF of some matrices?

Cubical/Algebra/IntegerMatrix/Diagonalization.agda Outdated Show resolved Hide resolved
Cubical/Algebra/IntegerMatrix/Smith/NormalForm.agda Outdated Show resolved Hide resolved
Cubical/Algebra/Matrix/RowTransformation.agda Outdated Show resolved Hide resolved
@@ -36,6 +36,8 @@ private
ℓ ℓ' : Level
A : Type ℓ

pattern one = suc zero
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wait, this should maybe not be here?

Copy link
Contributor Author

@kangrongji kangrongji Apr 5, 2022

Choose a reason for hiding this comment

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

Move it to Cubical.Data.FinData.

@mortberg
Copy link
Collaborator

mortberg commented Apr 5, 2022

PS: a lot of things were developed for 2x2 matrices. It would be good to generalize in the future, but this would involve a lot of work. If you're planning to go in this direction I recommend you to look at the mathematical components library (https://math-comp.github.io/), they have an extremely well-designed linear algebra library

@kangrongji
Copy link
Contributor Author

kangrongji commented Apr 5, 2022

I add a benchmark file Cubical.Experiments.IntegerMatrix so you can see some tests. It turns out that 2×2-matrices are OK. But even for 3×3-matrices, diagonalize only works for simple ones and smith doesn't work for any non-trivial ones (as far as I can say).

My wish is to develop some theories about finitely presented abelian groups, maybe in near future. Design pattern is what I need admittedly. I felt difficult in just organizing these things well :) I'll see if I could learn from math-comp, thanks for that suggestion!

@kangrongji kangrongji requested a review from mortberg April 5, 2022 13:38
Copy link
Collaborator

@mortberg mortberg left a comment

Choose a reason for hiding this comment

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

Just a small suggestion, then I would be happy to merge.

Strange that it's so slow. Maybe it would all be faster with VecMatrix? (I'm not suggesting that you implement that, but I'm just curious where the inefficiency might come from)

makeMat3×3 _ _ _ _ _ a12 _ _ _ one two = a12
makeMat3×3 _ _ _ _ _ _ a20 _ _ two zero = a20
makeMat3×3 _ _ _ _ _ _ _ a21 _ two one = a21
makeMat3×3 _ _ _ _ _ _ _ _ a22 two two = a22
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@kangrongji kangrongji Apr 6, 2022

Choose a reason for hiding this comment

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

I tried before. The result performs a bit slower than this direct definition, so I decided to use this one. Though it seems somewhat silly...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah ok, fair enough!

Copy link
Member

Choose a reason for hiding this comment

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

Maybe should be commented :/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ice1000 I'll comment it in future PR :P

@kangrongji
Copy link
Contributor Author

kangrongji commented Apr 6, 2022

Probably. One thing I've noticed is, if one only runs reduction step by step, the total time cost seems much less than running the whole function. (I've tried this months before, not contained in the benchmark file.) So I think if we can force Agda to fully normalize the result to numerals in each step, the function could be more efficient (I don't know whether Agda supports this or how to do that). Sadly I'm no expert of implementation details of Agda, so maybe it's just my non-sense.

@kangrongji kangrongji requested a review from mortberg April 6, 2022 12:26
@mortberg mortberg merged commit 53dbe5c into agda:master Apr 6, 2022
@kangrongji kangrongji deleted the smithexistence branch April 6, 2022 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants