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

fix(algebra/category): make has_coe_to_sort instances for bundled categories reducible #2290

Merged
merged 7 commits into from
Apr 1, 2020

Conversation

semorrison
Copy link
Collaborator

See discussion at https://leanprover.zulipchat.com/#narrow/stream/113488-general/topic/unbearable.20unhappiness.20of.20.60simp.60.20not.20working

I have added a library note explaining this issue, and put in two examples that ensure correct behaviour.

Copy link
Member

@gebner gebner left a comment

Choose a reason for hiding this comment

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

Should we add a linter for this?

src/algebra/category/CommRing/basic.lean Outdated Show resolved Hide resolved
src/algebra/category/CommRing/basic.lean Show resolved Hide resolved
Scott Morrison and others added 3 commits March 30, 2020 22:54
@semorrison
Copy link
Collaborator Author

Should we add a linter for this?

... let's see if this is actually the right course before we invest that level of energy. :-)

…munity/mathlib into has_coe_to_sort_reducible
@urkud
Copy link
Member

urkud commented Mar 30, 2020

Sorry, I'm very far from being an expert in reducibility & Lean... I remember that it caused problems when we did the refactoring of concrete categories, but I don't remember any details.

@semorrison semorrison removed the request for review from urkud March 30, 2020 22:35
@semorrison semorrison added the awaiting-review The author would like community review of the PR label Mar 30, 2020
Copy link
Member

@gebner gebner left a comment

Choose a reason for hiding this comment

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

You've already written this in the "locally reducible category instances" library note:

It's especially important that the has_coe_to_sort instance not contain an extra id as we want the semiring ↥R instance to also apply to semiring R.α (it seems to be impractical to guarantee that we always access R.α through the coercion rather than directly).

This here is exactly the issue we're having with (0 : R) = (0 : R.α) not being true modulo reducible definitional equality, and hence simp failing to apply obvious lemmas. So I think the change in this PR is the right way to go and also consistent with the existing design.

Another reason to make the has_coe_to_sort instances reducible is if we ever want to apply the category theory library to actual structures. For examples, if you had an element of the bundled integers x : IntRing, then you'd want to use it as an element of the unbundled integers x : ℤ without the simplifier complaining (i.e. ↥IntRing should be reducibly-definitionally equal to ).

@@ -44,6 +41,13 @@ to functions, for example. It's especially important that the `has_coe_to_sort`
instance not contain an extra `id` as we want the `semiring ↥R` instance to
also apply to `semiring R.α` (it seems to be impractical to guarantee that
we always access `R.α` through the coercion rather than directly).

TODO: Probably @[derive] should be able to create instances of the
Copy link
Member

@gebner gebner Mar 31, 2020

Choose a reason for hiding this comment

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

BTW, another option that works is:

instance : concrete_category SemiRing :=
(by apply_instance : concrete_category (bundled semiring))

@gebner gebner 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 Mar 31, 2020
@rwbarton
Copy link
Collaborator

Should we add a linter for this?

What would the linter do? Check that all has_coe_to_sort instances are reducible? Why stop there?

@gebner
Copy link
Member

gebner commented Mar 31, 2020

What would the linter do? Check that all has_coe_to_sort instances are reducible?

Since that is the most common problem here, yes.

Why stop there?

What else would you want to check?

@rwbarton
Copy link
Collaborator

How about that all definitions are reducible? That would solve even more problems with simp, right? :)

I think this change is okay but I'm not convinced we are fixing the right problem at the right level. Historically in category theory (e.g. with comp_id) we tended to accept that simp just doesn't do all the things one would want, rather than trying to force the issue by marking a lot of things reducible.

@mergify mergify bot merged commit 67e7f90 into master Apr 1, 2020
@gebner
Copy link
Member

gebner commented Apr 1, 2020

How about that all definitions are reducible? That would solve even more problems with simp, right? :)

This is a good question. Marking some definitions as reducible can actually be counter-productive. For example, simp lemmas such as f ∘ symm f = id don't work (reliably) because (∘) is reducible. Then f ∘ symm f unfolds to λ x, f (symm f x) and you suddenly need higher-order matching.

Another reason is that a definition might unfold to something "more complex". We can probably write reliable simplification lemmas for the simple definition. But this becomes much harder once it is unfolded and perhaps simplified so that it no longer matches the original definition.

I don't think the instances in this PR are problematic in this sense.

Historically in category theory (e.g. with comp_id) we tended to accept that simp just doesn't do all the things one would want

I believe it is worthwhile to make simp work.

but I'm not convinced we are fixing the right problem at the right level

I think the approach of marking definitions as reducible is the common solution to this kind of problem. If you want the simplifier to unfold a definition during matching, then you typically mark it as reducible.

I also thought about adding this simp lemma (and fixing bundled.map so that it uses R instead of R.α):

@[simp]
lemma bundled.α_eq_coe (c) (R : bundled c) : R.α = R := rfl

Alas, this doesn't work because the simplifier cannot rewrite the α in has_zero.zero α. This is because α occurs in the return type of has_zero.zero, and the simplifier can only do rewrites that preserve the type of an expression.

@semorrison
Copy link
Collaborator Author

Just noting that in Lean 3.8 we should be able to reverse this change. #169 there solves this problem.

@robertylewis robertylewis deleted the has_coe_to_sort_reducible branch April 9, 2020 15:23
bors bot pushed a commit that referenced this pull request Apr 11, 2020
Now that Lean 3.8 has arrived, we can essentially revert #2290, but leave in the examples verifying that everything still works.

Lovely!
anrddh pushed a commit to anrddh/mathlib that referenced this pull request May 15, 2020
…egories reducible (leanprover-community#2290)

* fix(algebra/category): make has_coe_to_sort instances for bundled categories reducible

* fix library notes

* fix import

* Update src/algebra/category/CommRing/basic.lean

Co-Authored-By: Gabriel Ebner <gebner@gebner.org>

* fix notes

Co-authored-by: Scott Morrison <scott.morrison@anu.edu.au>
Co-authored-by: Gabriel Ebner <gebner@gebner.org>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
anrddh pushed a commit to anrddh/mathlib that referenced this pull request May 15, 2020
…prover-community#2389)

Now that Lean 3.8 has arrived, we can essentially revert leanprover-community#2290, but leave in the examples verifying that everything still works.

Lovely!
anrddh pushed a commit to anrddh/mathlib that referenced this pull request May 16, 2020
…egories reducible (leanprover-community#2290)

* fix(algebra/category): make has_coe_to_sort instances for bundled categories reducible

* fix library notes

* fix import

* Update src/algebra/category/CommRing/basic.lean

Co-Authored-By: Gabriel Ebner <gebner@gebner.org>

* fix notes

Co-authored-by: Scott Morrison <scott.morrison@anu.edu.au>
Co-authored-by: Gabriel Ebner <gebner@gebner.org>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
anrddh pushed a commit to anrddh/mathlib that referenced this pull request May 16, 2020
…prover-community#2389)

Now that Lean 3.8 has arrived, we can essentially revert leanprover-community#2290, but leave in the examples verifying that everything still works.

Lovely!
cipher1024 pushed a commit to cipher1024/mathlib that referenced this pull request Mar 15, 2022
…egories reducible (leanprover-community#2290)

* fix(algebra/category): make has_coe_to_sort instances for bundled categories reducible

* fix library notes

* fix import

* Update src/algebra/category/CommRing/basic.lean

Co-Authored-By: Gabriel Ebner <gebner@gebner.org>

* fix notes

Co-authored-by: Scott Morrison <scott.morrison@anu.edu.au>
Co-authored-by: Gabriel Ebner <gebner@gebner.org>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
cipher1024 pushed a commit to cipher1024/mathlib that referenced this pull request Mar 15, 2022
…prover-community#2389)

Now that Lean 3.8 has arrived, we can essentially revert leanprover-community#2290, but leave in the examples verifying that everything still works.

Lovely!
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