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

feat(category_theory): add reassoc annotations #1558

Merged
merged 13 commits into from Oct 22, 2019
Merged

feat(category_theory): add reassoc annotations #1558

merged 13 commits into from Oct 22, 2019

Conversation

cipher1024
Copy link
Collaborator

TO CONTRIBUTORS:

Make sure you have:

  • reviewed and applied the coding style: coding, naming
  • reviewed and applied the documentation requirements
  • for tactics:
  • make sure definitions and lemmas are put in the right files
  • make sure definitions and lemmas are not redundant

If this PR is related to a discussion on Zulip, please include a link in the discussion.

For reviewers: code review check list

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.

Looks good to me... but I'm a metamateur...

@robertylewis
Copy link
Member

Definitions need docstrings. What does this actually change? I'm surprised to see the attribute added with no proofs affected.

@robertylewis robertylewis added the needs-documentation This PR is missing required documentation label Oct 16, 2019
@semorrison
Copy link
Collaborator

Okay, my take on this PR is that the real content is a new tactic, [reassoc] which tweaks a given hypothesis so that it can be used even when things aren't associated correctly.

Along the way, the PR adds a handful of correct, harmless, not-immediately necessary @[reassoc] attributes on existing lemmas, and also correctly removes some unused arguments in other lemmas.

Under the hood, it does some refactoring (with no change in behaviour) of the machinery for creating reassoc lemmas.

@cipher1024, is this a correct summary? Have I missed anything?

@semorrison
Copy link
Collaborator

@cipher1024, I added some documentation suggestions, but I'll let you merge them if you like them.

@semorrison semorrison added awaiting-author A reviewer has asked the author a question or requested changes and removed needs-documentation This PR is missing required documentation labels Oct 17, 2019
cipher1024 and others added 3 commits October 17, 2019 00:38
Co-Authored-By: Scott Morrison <scott@tqft.net>
Co-Authored-By: Scott Morrison <scott@tqft.net>
Co-Authored-By: Scott Morrison <scott@tqft.net>
@cipher1024
Copy link
Collaborator Author

Thanks!

@semorrison
Copy link
Collaborator

We also need some test cases. I'm not sure that the ! mode is useful enough to warrant adding.

@cipher1024
Copy link
Collaborator Author

I use it more than I don't use it. I must admit that I don't like keeping such a complicated assumption in my context though. Maybe I should try reworking it so that I can write rw [# h] where # creates a reassoc proof out h but without adding it to the context. That would work nicely with simp too.

@jcommelin
Copy link
Member

Hi that sounds like fun (-; But I'm also worried a bit.
While really short and convenient it's also an incomprehensible sigil that really doesn't explain what's going on. Soon we will require beginners to have expertise in reading runes to decipher proofs...
Also... are you suggesting to generate the reassoc proof whenever it's needed? That seems like it will also be slower than the current situation.
My 2 cowry shells (-;

@cipher1024
Copy link
Collaborator Author

I don't think I would generate them whenever needed for lemmas, only assumptions. Here is an example where the notation is less cryptic:

namespace tactic

def Prop' {α} (β : Prop) (hh : α) := β

meta def foo_tac : tactic unit :=
do `(Prop' %%v %%h) ← target,
   (t,pr) ← prove_reassoc h,
   unify v t,
   exact pr

end tactic

def reassoc_of {α} (hh : α) {β} (x : tactic.Prop' β hh . tactic.foo_tac) : β := x

open category_theory
variables {C : Type} [category.{1} C]

example (X Y Z W : C) (x : X ⟶ Y) (y : Y ⟶ Z) (z z' : Z ⟶ W) (w : X ⟶ Z)
  (h : x ≫ y = w)
  (h' : y ≫ z = y ≫ z') :
  x ≫ y ≫ z = w ≫ z' :=
begin
  rw [h',reassoc_of h],
end

@cipher1024
Copy link
Collaborator Author

(from experience, doing reassoc on an assumption gets really clunky and makes the context less readable; discarding it immediately seems simpler and more readable to me)

@jcommelin
Copy link
Member

Aah, that seems nice.

@semorrison
Copy link
Collaborator

I like this example. What is the plan for this PR?

@semorrison semorrison added WIP Work in progress and removed awaiting-author A reviewer has asked the author a question or requested changes labels Oct 21, 2019
@cipher1024
Copy link
Collaborator Author

I think I would just merge it as is. What do you think @semorrison?

@semorrison semorrison added awaiting-author A reviewer has asked the author a question or requested changes and removed WIP Work in progress labels Oct 21, 2019
@semorrison semorrison 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-author A reviewer has asked the author a question or requested changes labels Oct 21, 2019
@mergify mergify bot merged commit 2b98d47 into master Oct 22, 2019
@mergify mergify bot deleted the add-reassoc branch October 22, 2019 06:42
anrddh pushed a commit to anrddh/mathlib that referenced this pull request May 15, 2020
…y#1558)

* feat(category_theory): add `reassoc` annotations

* Update reassoc_axiom.lean

* Update src/tactic/reassoc_axiom.lean

Co-Authored-By: Scott Morrison <scott@tqft.net>

* Update src/tactic/reassoc_axiom.lean

Co-Authored-By: Scott Morrison <scott@tqft.net>

* Update src/tactic/reassoc_axiom.lean

Co-Authored-By: Scott Morrison <scott@tqft.net>

* Update src/tactic/reassoc_axiom.lean

* Update src/tactic/reassoc_axiom.lean

* Update reassoc_axiom.lean

* Update tactics.lean

* Update tactics.md

* Update reassoc_axiom.lean
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