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 AlgebraicTopology.SimplicialObject #3302

Closed
wants to merge 54 commits into from

Conversation

Mathbin -> Mathlib
fix certain import statements
move "by" to end of line
add import to Mathlib.lean
Mathbin -> Mathlib
fix certain import statements
move "by" to end of line
add import to Mathlib.lean
Mathbin -> Mathlib
fix certain import statements
move "by" to end of line
add import to Mathlib.lean
Mathbin -> Mathlib
fix certain import statements
move "by" to end of line
add import to Mathlib.lean
Mathbin -> Mathlib
fix certain import statements
move "by" to end of line
add import to Mathlib.lean
@joelriou joelriou added the mathlib-port This is a port of a theory file from mathlib. label Apr 6, 2023
@semorrison semorrison added the blocked-by-other-PR This PR depends on another PR which is still in the queue. label Apr 6, 2023
@joelriou joelriou removed the WIP Work in progress label Apr 6, 2023
@semorrison semorrison added merge-conflict The PR has a merge conflict with master, and needs manual merging. and removed blocked-by-other-PR This PR depends on another PR which is still in the queue. labels Apr 6, 2023
@semorrison semorrison removed the merge-conflict The PR has a merge conflict with master, and needs manual merging. label Apr 6, 2023
@Parcly-Taxel Parcly-Taxel added the awaiting-review The author would like community review of the PR label Apr 6, 2023
SimplexCategoryᵒᵖ ⥤ C
#align category_theory.simplicial_object CategoryTheory.SimplicialObject

@[simps!]
Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit confused about all these simps! attributes. There was nothing similar in mathlib3 right? Are they strictly necessary, and would it perhaps be better to make SimplicialObject a reducible instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Without the reducible attribute, the simps! for the Category instance (and extra ext lemmas for morphisms) were necessary. I have made SimplicialObject reducible in the last commit.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, I'll ping @jcommelin , @semorrison and @adamtopaz to see if they're happy with the change.

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean, I'd prefer that we don't make it reducible. It's more work to set up here, unfortunately. We have a tendency to regret making things less opaque, however...

Copy link
Collaborator Author

@joelriou joelriou Apr 7, 2023

Choose a reason for hiding this comment

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

Ok. I have reverted back to the previous situation where SimplicialObject was not declared reducible.

Copy link
Member

Choose a reason for hiding this comment

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

I'll let someone else take over the review of this PR, but I'm not sure that simp lemmas unfolding the definition of a type is a particularly good idea either for the same reasons. Probably a lot of the simp lemmas that become accessible after unfolding definitions should be proved on SimplicialObject directly.

Copy link
Collaborator Author

@joelriou joelriou Apr 7, 2023

Choose a reason for hiding this comment

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

Indeed, a lemma like

theorem SimplicialObject.δ_naturality {X' X : SimplicialObject C} (f : X ⟶ X') {n : ℕ} (i : Fin (n + 2)) :
    X.δ i ≫ f.app (op [n]) = f.app (op [n + 1]) ≫ X'.δ i

is a particular case of NatTrans.naturality, and it is stated as a simp lemma that is specific to SimplicialObject.

Copy link
Member

Choose a reason for hiding this comment

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

simps! currently produces simp lemmas unfolding the definition of Hom on all of these instances. I have a commit locally that replaces simps! with simps! id_app comp_app or similar to avoid unfolding the definition of Hom. The whole file builds without any other changes. Let me know if you think I should push this. Maybe @semorrison has an opinion on that, I'm not too familiar with CategoryTheory.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, a bit of a tangle here. I have a PR open on mathlib proposing to adjust the simps projections for category so we don't unfold Hom. I should have got that done and forwarded port faster...

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, I propose that I will quickly forward port that PR, and then merge that into this before we proceed?

@ChrisHughes24 ChrisHughes24 added WIP Work in progress and removed awaiting-review The author would like community review of the PR labels Apr 10, 2023
@semorrison semorrison added blocked-by-other-PR This PR depends on another PR which is still in the queue. and removed blocked-by-other-PR This PR depends on another PR which is still in the queue. labels Apr 10, 2023
@semorrison
Copy link
Contributor

@joelriou joelriou added awaiting-review The author would like community review of the PR and removed WIP Work in progress labels Apr 10, 2023
@ChrisHughes24
Copy link
Member

bors merge

@github-actions github-actions bot 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 Apr 11, 2023
bors bot pushed a commit that referenced this pull request Apr 11, 2023
@bors
Copy link

bors bot commented Apr 11, 2023

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title feat: port AlgebraicTopology.SimplicialObject [Merged by Bors] - feat: port AlgebraicTopology.SimplicialObject Apr 11, 2023
@bors bors bot closed this Apr 11, 2023
@bors bors bot deleted the port/AlgebraicTopology.SimplicialObject branch April 11, 2023 12:42
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