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: port CategoryTheory.Comma #2260

Closed
wants to merge 7 commits into from

Conversation

joelriou
Copy link
Collaborator


Open in Gitpod

@joelriou joelriou added the WIP Work in progress label Feb 13, 2023
@joelriou joelriou added awaiting-review The author would like community review of the PR and removed WIP Work in progress labels Feb 14, 2023
@jcommelin jcommelin added the mathlib-port This is a port of a theory file from mathlib. label Feb 14, 2023
Comment on lines 66 to 67
left : A --:= by obviously
right : B --:= by obviously
Copy link
Member

Choose a reason for hiding this comment

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

Can you use by aesop_cat here? That is now typically used as replacement for obviously/tidy.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I know, but I am not sure what by aesop_cat would do here because left/right are data, not Prop fields. Would it have any effect at all?!

Copy link
Contributor

Choose a reason for hiding this comment

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

Surprising that there was . obviously there in the first place. I think Scott got a bit carried away. I would suggest just deleting these comments, and leaving a porting note (that the Lean 3 version did not really make sense in the first place). Also in CommaMorphism below, except for the equational field w.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have just PRed this leanprover-community/mathlib#18440

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually, it turns out that these by obviously were necessary in mathlib3. The test mathlib PR just above does not compile. The reason is that for (co)structured_arrow, morphisms between initial/terminal objects could be filled automatically by obviously. Then, I have added by aesop_cat in this mathlib4 PR, and we may hope for the best that when we have limits in mathlib4, aesop_cat will be able to fill the obvious fields in (co)structured_arrow .

Copy link
Contributor

Choose a reason for hiding this comment

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

I would just add explicit terms wherever needed in mathlib 3; they can't be too complicated right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These terms are easy. Doing so more or less forces the user to use the explicit constructors like structured_arrow.mk and structured_arrow.hom_mk (rather than providing the relevant fields and relying on automation for the rest). This should eventually make the code cleaner, and easier to maintain/port.

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

@semorrison semorrison added ready-to-merge This PR has been sent to bors. and removed awaiting-review The author would like community review of the PR labels Feb 14, 2023
bors bot pushed a commit that referenced this pull request Feb 14, 2023
@bors
Copy link

bors bot commented Feb 14, 2023

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title feat: port CategoryTheory.Comma [Merged by Bors] - feat: port CategoryTheory.Comma Feb 14, 2023
@bors bors bot closed this Feb 14, 2023
@bors bors bot deleted the category-theory-comma branch February 14, 2023 11:57
mo271 pushed a commit that referenced this pull request Feb 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mathlib-port This is a port of a theory file from mathlib. ready-to-merge This PR has been sent to bors.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants