Skip to content
This repository was archived by the owner on Jul 24, 2024. It is now read-only.

chore(category_theory/concrete_category): take an instance, rather than extending, category#2195

Merged
mergify[bot] merged 12 commits intomasterfrom
concrete_no_extend
Apr 2, 2020
Merged

chore(category_theory/concrete_category): take an instance, rather than extending, category#2195
mergify[bot] merged 12 commits intomasterfrom
concrete_no_extend

Conversation

@kim-em
Copy link
Copy Markdown
Collaborator

@kim-em kim-em commented Mar 19, 2020

Commit Message

chore(category_theory/concrete_category): take an instance, rather than extending, category (#2195)

Our current design for concrete_category has it extend category.

This PR changes that so that is takes an instance, instead.

I want to do this because I ran into a problem defining concrete_monoidal_category, which is meant to be a monoidal category, whose faithful functor to Type is lax monoidal. (The prime example here is Module R, where there's a map F(X) \otimes F(Y) \to F(X \otimes Y), but not the other way: here F(X) \otimes F(Y) is just the monoidal product in Type, i.e. cartesian product, and the function here is (x, y) \mapsto x \otimes y.)

Now, monoidal_category does not extend category, but instead takes a typeclass, so with the old design concrete_monoidal_category was going to be a Frankenstein, extending concrete_category and taking a [monoidal_category] type class. However, when 3.7 landed this went horribly wrong, and even defining concrete_monoidal_category caused an unbounded typeclass search.

So.... I've made everything simpler, and just not used extends at all. It's all just typeclasses. This makes some places a bit more verbose, as we have to summon lots of separate typeclasses, but besides that everything works smoothly.

(Note, this PR makes the change to concrete_category, but does not yet introduce concrete_monoidal_category.)

Copy link
Copy Markdown
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.

@jcommelin
Copy link
Copy Markdown
Member

I would like to hear from others what they think of this change (@urkud @rwbarton)

@kim-em kim-em added the awaiting-review The author would like community review of the PR label Mar 20, 2020
@rwbarton
Copy link
Copy Markdown
Collaborator

Fine with me--I'm not much of a fan of extends anyways, though it should also be said that I don't really ever use concrete_category either so I don't have much of an opinion about it.

@rwbarton
Copy link
Copy Markdown
Collaborator

There are some not obviously related changes involving of_self_iso here; are those supposed to be included?

@jcommelin jcommelin 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 Apr 1, 2020
@jcommelin
Copy link
Copy Markdown
Member

I like the new of_self_iso name better... so I've kicked this on the queue.

@mergify mergify bot merged commit 313cc2f into master Apr 2, 2020
@mergify mergify bot deleted the concrete_no_extend branch April 2, 2020 08:18
butterthebuddha pushed a commit to butterthebuddha/mathlib that referenced this pull request May 15, 2020
…an extending, category (leanprover-community#2195)

chore(category_theory/concrete_category): take an instance, rather than extending, category (leanprover-community#2195)

Our current design for `concrete_category` has it extend `category`.

This PR changes that so that is takes an instance, instead.

I want to do this because I ran into a problem defining `concrete_monoidal_category`, which is meant to be a monoidal category, whose faithful functor to Type is lax monoidal. (The prime example here is `Module R`, where there's a map `F(X) \otimes F(Y) \to F(X \otimes Y)`, but not the other way: here `F(X) \otimes F(Y)` is just the monoidal product in Type, i.e. cartesian product, and the function here is `(x, y) \mapsto x \otimes y`.)

Now, `monoidal_category` does not extend `category`, but instead takes a typeclass, so with the old design `concrete_monoidal_category` was going to be a Frankenstein, extending `concrete_category` and taking a `[monoidal_category]` type class. However, when 3.7 landed this went horribly wrong, and even defining `concrete_monoidal_category` caused an unbounded typeclass search.

So.... I've made everything simpler, and just not used `extends` at all. It's all just typeclasses. This makes some places a bit more verbose, as we have to summon lots of separate typeclasses, but besides that everything works smoothly.

(Note, this PR makes the change to `concrete_category`, but does not yet introduce `concrete_monoidal_category`.)
butterthebuddha pushed a commit to butterthebuddha/mathlib that referenced this pull request May 16, 2020
…an extending, category (leanprover-community#2195)

chore(category_theory/concrete_category): take an instance, rather than extending, category (leanprover-community#2195)

Our current design for `concrete_category` has it extend `category`.

This PR changes that so that is takes an instance, instead.

I want to do this because I ran into a problem defining `concrete_monoidal_category`, which is meant to be a monoidal category, whose faithful functor to Type is lax monoidal. (The prime example here is `Module R`, where there's a map `F(X) \otimes F(Y) \to F(X \otimes Y)`, but not the other way: here `F(X) \otimes F(Y)` is just the monoidal product in Type, i.e. cartesian product, and the function here is `(x, y) \mapsto x \otimes y`.)

Now, `monoidal_category` does not extend `category`, but instead takes a typeclass, so with the old design `concrete_monoidal_category` was going to be a Frankenstein, extending `concrete_category` and taking a `[monoidal_category]` type class. However, when 3.7 landed this went horribly wrong, and even defining `concrete_monoidal_category` caused an unbounded typeclass search.

So.... I've made everything simpler, and just not used `extends` at all. It's all just typeclasses. This makes some places a bit more verbose, as we have to summon lots of separate typeclasses, but besides that everything works smoothly.

(Note, this PR makes the change to `concrete_category`, but does not yet introduce `concrete_monoidal_category`.)
cipher1024 pushed a commit to cipher1024/mathlib that referenced this pull request Mar 15, 2022
…an extending, category (leanprover-community#2195)

chore(category_theory/concrete_category): take an instance, rather than extending, category (leanprover-community#2195)

Our current design for `concrete_category` has it extend `category`.

This PR changes that so that is takes an instance, instead.

I want to do this because I ran into a problem defining `concrete_monoidal_category`, which is meant to be a monoidal category, whose faithful functor to Type is lax monoidal. (The prime example here is `Module R`, where there's a map `F(X) \otimes F(Y) \to F(X \otimes Y)`, but not the other way: here `F(X) \otimes F(Y)` is just the monoidal product in Type, i.e. cartesian product, and the function here is `(x, y) \mapsto x \otimes y`.)

Now, `monoidal_category` does not extend `category`, but instead takes a typeclass, so with the old design `concrete_monoidal_category` was going to be a Frankenstein, extending `concrete_category` and taking a `[monoidal_category]` type class. However, when 3.7 landed this went horribly wrong, and even defining `concrete_monoidal_category` caused an unbounded typeclass search.

So.... I've made everything simpler, and just not used `extends` at all. It's all just typeclasses. This makes some places a bit more verbose, as we have to summon lots of separate typeclasses, but besides that everything works smoothly.

(Note, this PR makes the change to `concrete_category`, but does not yet introduce `concrete_monoidal_category`.)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

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.

3 participants