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] - chore(linear_algebra/tensor_product): remove @[ext] tag from tensor_product.mk_compr₂_inj #8868

Closed
wants to merge 6 commits into from

Conversation

dupuisf
Copy link
Collaborator

@dupuisf dupuisf commented Aug 25, 2021

This PR removes the @[ext] tag from tensor_product.mk_compr₂_inj and readds it locally only where it is needed. This is a workaround for the issue discussed in this Zulip thread: basically, when ext tries to apply this lemma to linear maps, it fails only after a very long typeclass search. While this problem is already present to some extent in current mathlib, it is exacerbated by the upcoming generalization of linear maps to semilinear maps.

In addition to this change, a few individual uses of ext have been replaced by a manual application of the relevant ext lemma(s) for performance reasons.

For discoverability, the lemma tensor_product.mk_compr₂_inj is renamed to tensor_product.ext and the former tensor_product.ext to tensor_product.ext'.


Open in Gitpod

@dupuisf dupuisf changed the title Semilinear extfix chore(*): remove @[ext] tag from tensor_product.mk_compr₂_inj Aug 25, 2021
@dupuisf dupuisf added the awaiting-review The author would like community review of the PR label Aug 25, 2021
@dupuisf dupuisf changed the title chore(*): remove @[ext] tag from tensor_product.mk_compr₂_inj chore(linear_algebra/tensor_product): remove @[ext] tag from tensor_product.mk_compr₂_inj Aug 25, 2021
@eric-wieser
Copy link
Member

eric-wieser commented Aug 31, 2021

Can we fix this by adjusting the priority instead?

Or perhaps by locally removing the attribute where it causes timeouts?

I don't really like the idea of removing this ext lemma (which for reference was added in #6105), because the chance of someone who needs it finding it is very small.

@dupuisf
Copy link
Collaborator Author

dupuisf commented Aug 31, 2021

Can we fix this by adjusting the priority instead?

Or perhaps by locally removing the attribute where it causes timeouts?

I don't really like the idea of removing this ext lemma, because the chance of someone who needs it finding it is very small.

I guess that could be another option, but my concern is that it seems to cause performance problems in fairly generic situations (i.e. calling ext on a linear map) -- this might also be hard to debug for someone who is not aware of the issue. Maybe we could rename the lemma to tensor_product.ext to make it easier to find?

@eric-wieser
Copy link
Member

eric-wieser commented Aug 31, 2021

I wonder if replacing [] binders with {} binders in mk_compr₂_inj would speed things up? The statement is currently

theorem tensor_product.mk_compr₂_inj {R : Type u_1} [comm_semiring R] {M : Type u_4} {N : Type u_5} {P : Type u_6}
  [add_comm_monoid M] [add_comm_monoid N] [add_comm_monoid P] [module R M] [module R N] [module R P]
  {g h : M ⊗ N →ₗ[R] P} (H : (tensor_product.mk R M N).compr₂ g = (tensor_product.mk R M N).compr₂ h) :
g = h

but perhaps it would be faster as

theorem tensor_product.mk_compr₂_inj {R : Type u_1} {_ : comm_semiring R} {M : Type u_4} {N : Type u_5} {P : Type u_6}
  {add_comm_monoid M : _} {add_comm_monoid N : _} {add_comm_monoid P : _} {_ : module R M} {_ : module R N} {_ : module R P}
  {g h : M ⊗ N →ₗ[R] P} (H : (tensor_product.mk R M N).compr₂ g = (tensor_product.mk R M N).compr₂ h) :
g = h

I think it would be worth creating a Zulip thread about the performance here to see if anyone can work out the underlying cause. Edit: I posted in the zulip thread.

@dupuisf
Copy link
Collaborator Author

dupuisf commented Aug 31, 2021

I wonder if replacing [] binders with {} binders in mk_compr₂_inj would speed things up?

I actually tried that first :-) It made no difference on the compile time.

@github-actions github-actions bot added the merge-conflict Please `git merge origin/master` then a bot will remove this label. label Sep 6, 2021
@github-actions github-actions bot removed the merge-conflict Please `git merge origin/master` then a bot will remove this label. label Sep 6, 2021
@dupuisf
Copy link
Collaborator Author

dupuisf commented Sep 7, 2021

I have now renamed tensor_product.mk_compr₂_inj to tensor_product.ext and the former tensor_product.ext to tensor_product.ext'.

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.

Thanks 🎉

bors merge

@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 Sep 7, 2021
bors bot pushed a commit that referenced this pull request Sep 7, 2021
…r_product.mk_compr₂_inj` (#8868)

This PR removes the `@[ext]` tag from `tensor_product.mk_compr₂_inj` and readds it locally only where it is needed. This is a workaround for the issue discussed [in this Zulip thread](https://leanprover.zulipchat.com/#narrow/stream/113488-general/topic/odd.20repeated.20type.20class.20search): basically, when `ext` tries to apply this lemma to linear maps, it fails only after a very long typeclass search. While this problem is already present to some extent in current mathlib, it is exacerbated by the [upcoming generalization of linear maps to semilinear maps](https://leanprover.zulipchat.com/#narrow/stream/113488-general/topic/Semilinear.20maps).

In addition to this change, a few individual uses of `ext` have been replaced by a manual application of the relevant ext lemma(s) for performance reasons.

For discoverability, the lemma `tensor_product.mk_compr₂_inj` is renamed to `tensor_product.ext` and the former `tensor_product.ext` to `tensor_product.ext'`.
@bors
Copy link

bors bot commented Sep 7, 2021

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title chore(linear_algebra/tensor_product): remove @[ext] tag from tensor_product.mk_compr₂_inj [Merged by Bors] - chore(linear_algebra/tensor_product): remove @[ext] tag from tensor_product.mk_compr₂_inj Sep 7, 2021
@bors bors bot closed this Sep 7, 2021
@bors bors bot deleted the semilinear-extfix branch September 7, 2021 08:00
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

3 participants