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(matrix/kronecker): Add the Kronecker product #8560

Closed
wants to merge 10 commits into from

Conversation

eric-wieser
Copy link
Member

@eric-wieser eric-wieser commented Aug 5, 2021

Largely derived from #8152, avoiding the complexity of introducing algebra_maps.

This introduces an abstraction kronecker_map, which is used to support both mul and tmul without having to redo any proofs.


Open in Gitpod

Present in this PR but not #8152:

  • Lemmas about map and transpose
  • The name kronecker instead of kronecker_prod; prod usually means an n-ary product in mathlib. not *. I am happy to change this.
  • Arithmetic lemmas specializd to ⊗ₖ like (A₁ + A₂) ⊗ₖ B = A₁ ⊗ₖ B + A₂ ⊗ₖ B
  • The tensor product version A ⊗ₖₜ[R] B (which was almost but not quite in the original PR

Present in #8152 but not this PR (left for a follow-up):

  • A kronecker_biprod equivalent to (algebra.linear_map _ _).map_matrix A ⊗ₖ (algebra.linear_map _ _).map_matrix B
  • Lemmas about associativity

Things I particularly want feedback on:

  • Is the notation reasonable?
  • Are the names reasonable?
  • Should the tensor product versions go in a different file?

Co-authored-by: faenuccio <filippo.nuccio@univ-st-etienne.fr>
@eric-wieser eric-wieser added the awaiting-review The author would like community review of the PR label Aug 5, 2021
@faenuccio
Copy link
Collaborator

faenuccio commented Aug 6, 2021

I think it is a very good file, both concerning notations and definitions. I have some comments which I will insert below, but I believe that associativity is really necessary (it certainly is in LTE). Creating a different file for the tensor products seems too much (to me), I would rather consider creating a different file (probably in linear_algebra) showing that the Kronecker product corresponds to the tensor product of linear maps when fixing basis.


## Specializations

* `matrix.kronecker`: An alias of `kronecker_map (*)`. Prefer using the notation.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would call this kronecker_prod or kronecker_mul rather than simply kronecker.

Copy link
Member Author

Choose a reason for hiding this comment

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

My only fear with kronecker_mul is that we end up with the lemma mul_kronecker_mul_mul which is confusing!

Copy link
Member Author

Choose a reason for hiding this comment

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

Another option would be kmul to match tmul, smul.

Copy link
Collaborator

@faenuccio faenuccio Aug 6, 2021

Choose a reason for hiding this comment

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

Good option! My point is that kronecker is a name for a thousand things in maths (from Wikipedia: Kronecker limit formula, Kronecker's congruence, Kronecker delta, Kronecker comb, Kronecker symbol, Kronecker product, Kronecker's method for factorizing polynomials, Kronecker substitution, Kronecker's theorem in number theory, Kronecker's lemma, and Eisenstein–Kronecker numbers) and I'd rather help a user specifying this is the product.

Copy link
Member

Choose a reason for hiding this comment

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

@faenuccio but it is namespaced to matrix.kronecker. I think that's sufficiently unambiguous, right?

@eric-wieser
Copy link
Member Author

Creating a different file for the tensor products seems too much (to me)

The reason I suggest this is that the tensor product version brings in an import of ring_theory.tensor_algebra, which probably pulls in a lot of imports. It's entirely plausible no one cares though.

@faenuccio
Copy link
Collaborator

Creating a different file for the tensor products seems too much (to me)

The reason I suggest this is that the tensor product version brings in an import of ring_theory.tensor_algebra, which probably pulls in a lot of imports. It's entirely plausible no one cares though.

Ok, mine was just an "organizational" point of view (in my mind, something substantial requires a new file, while things here seem so close to each other that this is not needed) but if you have stronger arguments I don't have any objection.

@eric-wieser
Copy link
Member Author

eric-wieser commented Aug 6, 2021

I believe that associativity is really necessary (it certainly is in LTE).

I agree, but I'd like to leave that to a follow up. In particular, I'm considering introducing a new Prop for that kind of statement:

/-- A property indicating that two matrices are equal modulo reindexing --/
def equal_mod_reindex (A : matrix l m α) (B : matrix n p α) (e : l ≃ n) (e' : m ≃ p) : Prop :=
A = B.minor e e'

notation A ` =ᵢ[`:50 e `, ` e' `] ` B:50 := equal_mod_reindex A B e e'

def equal_mod_reindex.eq {A : matrix l m α} {B : matrix n p α} {e : l ≃ n} {e' : m ≃ p}
  (h : A =ᵢ[e, e'] B) : A = B.minor e e' := h

@[symm]  -- TODO: add trans and refl
def equal_mod_reindex.symm {A : matrix l m α} {B : matrix n p α} {e : l ≃ n} {e' : m ≃ p}
  (h : A =ᵢ[e, e'] B) : B =ᵢ[e.symm, e'.symm] A :=
by convert ((matrix.reindex e.symm e'.symm).symm_apply_apply B).symm

def equal_mod_reindex_comm {A : matrix l m α} {B : matrix n p α} {e : l ≃ n} {e' : m ≃ p} :
  A =ᵢ[e, e'] B ↔ B =ᵢ[e.symm, e'.symm] A :=
⟨equal_mod_reindex.symm, equal_mod_reindex.symm⟩

lemma kronecker_map_comm (f : α → β → γ) (g : β → α → γ) (hcomm : ∀ a b, f a b = g b a)
  (A : matrix l m α) (B : matrix n p β) :
  kronecker_map f A B =ᵢ[prod_comm _ _, prod_comm _ _] kronecker_map g B A :=
ext $ λ i j, hcomm _ _

lemma kronecker_map_assoc (f : α → β' → γ') (g : β → γ → β') (f' : α' → γ → γ') (g' : α → β → α')
  (hassoc : ∀ a b c, f a (g b c) = f' (g' a b) c)
  (A : matrix l m α) (B : matrix n p β) (C : matrix l' m' γ):
  kronecker_map f A (kronecker_map g B C)
    =ᵢ[(prod_assoc _ _ _).symm, (prod_assoc _ _ _).symm]
      (kronecker_map f' (kronecker_map g' A B) C) :=
ext $ λ i j, hassoc _ _ _

@faenuccio
Copy link
Collaborator

I believe that associativity is really necessary (it certainly is in LTE).

I agree, but I'd like to leave that to a follow up. In particular, I'm considering introducing a new Prop for that kind of statement:

/-- A property indicating that two matrices are equal modulo reindexing --/
def equal_mod_reindex (A : matrix l m α) (B : matrix n p α) (e : l ≃ n) (e' : m ≃ p) : Prop :=
A = B.minor e e'

notation A ` =ᵢ[`:50 e `, ` e' `] ` B:50 := equal_mod_reindex A B e e'

def equal_mod_reindex.eq {A : matrix l m α} {B : matrix n p α} {e : l ≃ n} {e' : m ≃ p}
  (h : A =ᵢ[e, e'] B) : A = B.minor e e' := h

@[symm]  -- TODO: add trans and refl
def equal_mod_reindex.symm {A : matrix l m α} {B : matrix n p α} {e : l ≃ n} {e' : m ≃ p}
  (h : A =ᵢ[e, e'] B) : B =ᵢ[e.symm, e'.symm] A :=
by convert ((matrix.reindex e.symm e'.symm).symm_apply_apply B).symm

def equal_mod_reindex_comm {A : matrix l m α} {B : matrix n p α} {e : l ≃ n} {e' : m ≃ p} :
  A =ᵢ[e, e'] B ↔ B =ᵢ[e.symm, e'.symm] A :=
⟨equal_mod_reindex.symm, equal_mod_reindex.symm⟩

lemma kronecker_map_comm (f : α → β → γ) (g : β → α → γ) (hcomm : ∀ a b, f a b = g b a)
  (A : matrix l m α) (B : matrix n p β) :
  kronecker_map f A B =ᵢ[prod_comm _ _, prod_comm _ _] kronecker_map g B A :=
ext $ λ i j, hcomm _ _

lemma kronecker_map_assoc (f : α → β' → γ') (g : β → γ → β') (f' : α' → γ → γ') (g' : α → β → α')
  (hassoc : ∀ a b c, f a (g b c) = f' (g' a b) c)
  (A : matrix l m α) (B : matrix n p β) (C : matrix l' m' γ):
  kronecker_map f A (kronecker_map g B C)
    =ᵢ[(prod_assoc _ _ _).symm, (prod_assoc _ _ _).symm]
      (kronecker_map f' (kronecker_map g' A B) C) :=
ext $ λ i j, hassoc _ _ _

I see. Well, I guess that you will need a maintainer to verify and validate this PR. IMHO all this deserves to land in mathlib only once associativity is there, but of course someone more experienced than myself will decide. Same for kmul, bilinear vs linear and the other commit I proposed.

Copy link
Member

@jcommelin jcommelin left a comment

Choose a reason for hiding this comment

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

This looks like a nice abstraction! Thanks for the PR!

src/data/matrix/kronecker.lean Outdated Show resolved Hide resolved

## Specializations

* `matrix.kronecker`: An alias of `kronecker_map (*)`. Prefer using the notation.
Copy link
Member

Choose a reason for hiding this comment

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

@faenuccio but it is namespaced to matrix.kronecker. I think that's sufficiently unambiguous, right?

src/data/matrix/kronecker.lean Outdated Show resolved Hide resolved
src/data/matrix/kronecker.lean Outdated Show resolved Hide resolved
src/data/matrix/kronecker.lean Outdated Show resolved Hide resolved
A ⊗ₖ (B₁ + B₂) = A ⊗ₖ B₁ + A ⊗ₖ B₂ :=
kronecker_map_add_right _ mul_add _ _ _

lemma smul_kronecker [monoid R] [monoid α] [mul_action R α] [is_scalar_tower R α α]
Copy link
Member

Choose a reason for hiding this comment

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

Should this be a simp-lemma? Same for the next lemma.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think probably not, since we don't make the corresponding lemma for mul instead of kronecker simp.

src/data/matrix/kronecker.lean Outdated Show resolved Hide resolved
src/data/matrix/kronecker.lean Outdated Show resolved Hide resolved
@eric-wieser
Copy link
Member Author

@jcommelin, are you happy with the idea of renaming kronecker_tmul to ktmul and kronecker to kmul?

@jcommelin
Copy link
Member

@eric-wieser I think I would just keep the name matrix.kronecker. But if people think it is too long, then kmul is fine.

@eric-wieser
Copy link
Member Author

Alright, I'll leave the name as is and let someone who gets annoyed by the long name rename it later.

Copy link
Member

@jcommelin jcommelin left a comment

Choose a reason for hiding this comment

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

bors d+

@bors
Copy link

bors bot commented Aug 9, 2021

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

@github-actions github-actions bot added delegated The PR author may merge after reviewing final suggestions. and removed awaiting-review The author would like community review of the PR labels Aug 9, 2021
@eric-wieser
Copy link
Member Author

bors r+

@github-actions github-actions bot added the ready-to-merge All that is left is for bors to build and merge this PR. (Remember you need to say `bors r+`.) label Aug 9, 2021
bors bot pushed a commit that referenced this pull request Aug 9, 2021
Largely derived from #8152, avoiding the complexity of introducing `algebra_map`s.

This introduces an abstraction `kronecker_map`, which is used to support both `mul` and `tmul` without having to redo any proofs.



Co-authored-by: faenuccio <filippo.nuccio@univ-st-etienne.fr>
@bors
Copy link

bors bot commented Aug 9, 2021

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title feat(matrix/kronecker): Add the Kronecker product [Merged by Bors] - feat(matrix/kronecker): Add the Kronecker product Aug 9, 2021
@bors bors bot closed this Aug 9, 2021
@bors bors bot deleted the eric-wieser/kronecker branch August 9, 2021 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
delegated The PR author may merge after reviewing final suggestions. ready-to-merge All that is left is for bors to build and merge this PR. (Remember you need to say `bors r+`.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants