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(linear_algebra/tensor_product): tensor_product.map is bilinear in its two arguments #13608

Closed
wants to merge 21 commits into from

Conversation

antoinelab01
Copy link
Collaborator


Open in Gitpod

@antoinelab01 antoinelab01 changed the title feat(linear/algebra/tensor_product): tensor_product.map is bilinear in its two arguments feat(linear_algebra/tensor_product): tensor_product.map is bilinear in its two arguments Apr 22, 2022
Copy link
Member

@eric-wieser eric-wieser left a comment

Choose a reason for hiding this comment

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

One thing we could consider doing is redefining map directly as the bilinear version. Here's one way to do that:

-- this belongs in `bilinear_map.lean`
/-- A bilinear version of `linear_map.compl₁₂`. -/
def _root_.linear_map.lcompl₁₂ (f : P →ₗ[R] Q →ₗ[R] S) :
  (M →ₗ[R] P) →ₗ[R] (N →ₗ[R] Q) →ₗ[R] (M →ₗ[R] N →ₗ[R] S) :=
begin
  refine linear_map.mk₂ R f.compl₁₂ _ _ _ _; sorry
end

/-- The tensor product of a pair of linear maps between modules, bilinear in both maps. -/
def map : (M →ₗ[R] P) →ₗ[R] (N →ₗ[R] Q) →ₗ[R] (M ⊗[R] N →ₗ[R] P ⊗[R] Q) :=
(mk R P Q).lcompl₁₂.compr₂ $ uncurry R M N (P ⊗ Q)

If nothing breaks, then I think that would maybe be a good idea. Might be worth waiting for other reviewer opinions

antoinelab01 and others added 2 commits April 22, 2022 11:15
Co-authored-by: Eric Wieser <wieser.eric@gmail.com>
@antoinelab01 antoinelab01 added the awaiting-review The author would like community review of the PR label Apr 22, 2022
src/linear_algebra/dual.lean Outdated Show resolved Hide resolved
src/linear_algebra/tensor_product.lean Outdated Show resolved Hide resolved
src/linear_algebra/tensor_product.lean Outdated Show resolved Hide resolved
src/linear_algebra/dual.lean Outdated Show resolved Hide resolved
src/linear_algebra/dual.lean Outdated Show resolved Hide resolved
@jcommelin jcommelin added awaiting-author A reviewer has asked the author a question or requested changes and removed awaiting-review The author would like community review of the PR labels Apr 25, 2022
antoinelab01 and others added 5 commits April 25, 2022 11:47
Co-authored-by: Johan Commelin <johan@commelin.net>
Co-authored-by: Johan Commelin <johan@commelin.net>
Co-authored-by: Johan Commelin <johan@commelin.net>
Co-authored-by: Johan Commelin <johan@commelin.net>
Co-authored-by: Johan Commelin <johan@commelin.net>
@antoinelab01 antoinelab01 added awaiting-review The author would like community review of the PR and removed awaiting-author A reviewer has asked the author a question or requested changes labels Apr 25, 2022
Copy link
Member

@eric-wieser eric-wieser left a comment

Choose a reason for hiding this comment

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

What do you (or other reviewers) think about renaming to something along the lines of:

  • dual_tensor_dual_map to tensor_product.dual_distrib
  • dual_tensor_dual_inv_of_basis to dual.tensor_product_distrib_of_basis
  • dual_tensor_dual_equiv_of_basis to tensor_product.dual_distrib_equiv_of_basis

@antoinelab01
Copy link
Collaborator Author

What do you (or other reviewers) think about renaming to something along the lines of:

* `dual_tensor_dual_map` to `tensor_product.dual_distrib`

* `dual_tensor_dual_inv_of_basis` to `dual.tensor_product_distrib_of_basis`

* `dual_tensor_dual_equiv_of_basis` to `tensor_product.dual_distrib_equiv_of_basis`

Sounds fair, though don't it feels a bit weird to have a different prefix for the inverse function? Why not tensor_product.dual_distrib_inv_of_basis for the second one?

@antoinelab01
Copy link
Collaborator Author

One thing we could consider doing is redefining map directly as the bilinear version. Here's one way to do that:

-- this belongs in `bilinear_map.lean`
/-- A bilinear version of `linear_map.compl₁₂`. -/
def _root_.linear_map.lcompl₁₂ (f : P →ₗ[R] Q →ₗ[R] S) :
  (M →ₗ[R] P) →ₗ[R] (N →ₗ[R] Q) →ₗ[R] (M →ₗ[R] N →ₗ[R] S) :=
begin
  refine linear_map.mk₂ R f.compl₁₂ _ _ _ _; sorry
end

/-- The tensor product of a pair of linear maps between modules, bilinear in both maps. -/
def map : (M →ₗ[R] P) →ₗ[R] (N →ₗ[R] Q) →ₗ[R] (M ⊗[R] N →ₗ[R] P ⊗[R] Q) :=
(mk R P Q).lcompl₁₂.compr₂ $ uncurry R M N (P ⊗ Q)

If nothing breaks, then I think that would maybe be a good idea. Might be worth waiting for other reviewer opinions

One reason I see for not doing that is that map_bilinear should have R, M, N, P, Q as explicit variables, while map has them implicit, so changing map to the bilinear version would require adding these variables everywhere map is used which doesn't seem like a good idea.

@dupuisf
Copy link
Collaborator

dupuisf commented May 13, 2022

Looks good, thanks!

bors r+

@github-actions github-actions bot added ready-to-merge All that is left is for bors to build and merge this PR. (Remember you need to say `bors r+`.) and removed awaiting-review The author would like community review of the PR labels May 13, 2022
bors bot pushed a commit that referenced this pull request May 13, 2022
…n its two arguments (#13608)




Co-authored-by: antoinelab01 <66086247+antoinelab01@users.noreply.github.com>
@bors
Copy link

bors bot commented May 13, 2022

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title feat(linear_algebra/tensor_product): tensor_product.map is bilinear in its two arguments [Merged by Bors] - feat(linear_algebra/tensor_product): tensor_product.map is bilinear in its two arguments May 13, 2022
@bors bors bot closed this May 13, 2022
@bors bors bot deleted the trace branch May 13, 2022 02:53
bors bot pushed a commit that referenced this pull request May 17, 2022
…om is an equivalence (#13728)

- [x] depends on: #13608
- [x] depends on: #13733



Co-authored-by: antoinelab01 <66086247+antoinelab01@users.noreply.github.com>
bors bot pushed a commit that referenced this pull request May 17, 2022
…om is an equivalence (#13728)

- [x] depends on: #13608
- [x] depends on: #13733



Co-authored-by: antoinelab01 <66086247+antoinelab01@users.noreply.github.com>
bors bot pushed a commit that referenced this pull request May 17, 2022
…om is an equivalence (#13728)

- [x] depends on: #13608
- [x] depends on: #13733



Co-authored-by: antoinelab01 <66086247+antoinelab01@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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

4 participants