Skip to content

Determinant and Adjugate Matrix#1165

Open
FranziskusWiesnet wants to merge 8 commits into
agda:masterfrom
FranziskusWiesnet:master
Open

Determinant and Adjugate Matrix#1165
FranziskusWiesnet wants to merge 8 commits into
agda:masterfrom
FranziskusWiesnet:master

Conversation

@FranziskusWiesnet
Copy link
Copy Markdown

In the four new files, the determinant of a matrix over a commutative ring is introduced through Laplace expansion, and it is proven that the adjugate matrix multiplied by the matrix itself equals the determinant times the identity matrix.
Additionally, it is shown that the determinant is independent of the row or column chosen for expansion, along with a few other minor properties of the determinant.
I am happy to consider any improvements.

Comment thread dist-newstyle/cache/compiler Outdated
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This looks like accidentally committed files

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yes. I have removed it.

@felixwellen
Copy link
Copy Markdown
Collaborator

Thanks for opening a PR on this! Determinants would certainly be very useful to have.

Comment thread Cubical/Algebra/Determinat/Minor.agda Outdated
@@ -0,0 +1,194 @@
{-# OPTIONS --cubical #-}

module Minor where
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Module names should always be fully qualified, i.e. Cubical.Algebra.Determinant.Minor in this case.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thank you! I have fixed it.

CommRingR' : CommRingStr (R' .fst)
CommRingR' = commringstr 0r 1r _+_ _·_ -_ (CommRingStr.isCommRing (snd P'))

-- Definition of the minor factor
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I noticed that there is some redundancy in the Determinat (is that a typo?) .Base file. MF i is just (- 1r) ^ i (_^_ is defined in Cubical.Algebra.CommRing.Properties, and many of the properties of minor factor you prove are already proved there). And +Compat can by proved more simply by cong₂ _+_.

Copy link
Copy Markdown
Author

@FranziskusWiesnet FranziskusWiesnet Jan 15, 2025

Choose a reason for hiding this comment

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

Thank you! That could very well be the case. There is certainly a lot to improve here. When I wrote that, I had just started with Agda.

@felixwellen felixwellen self-assigned this Feb 7, 2025
felixwellen and others added 3 commits May 23, 2025 12:03
…c000fafb882e5b6046408f6993e0"

This reverts commit e7aee8949ba8708c0ffbdb8f8d21fbdb13af6153.
Some fixes for your PR on determinants
@mortberg
Copy link
Copy Markdown
Collaborator

What is the state of this PR? Has all comments been addressed so that it's ready for another round of reviews/merging?

@felixwellen
Copy link
Copy Markdown
Collaborator

I honestly don't remember - I think I had a discussion with @FranziskusWiesnet which might not have taken place here. At a quick glance I only see {-# OPTIONS --safe #-} which are all not necessary anymore.

@FranziskusWiesnet
Copy link
Copy Markdown
Author

Thanks for checking in! Since this PR has been open for quite some time, I do not remember all details either.
As far as I can see, Felix’s fixes have been merged and the CI currently passes. I can remove the remaining unnecessary {-# OPTIONS --safe #-} pragmas in a small commit.

Could someone start another round of review? I would be happy to address any remaining issues.

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.

4 participants