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] - refactor(topology/sheaves/*): Make sheaf condition a Prop #9607

Closed
wants to merge 2 commits into from

Conversation

justus-springer
Copy link
Collaborator

Make sheaf_condition into a Prop and redefine the type of sheaves on a topological space X as a subtype of (opens X)ᵒᵖ ⥤ C.


Previously, sheaves on a topological space were defined as a structure with a Type-valued field sheaf_condition. Meanwhile, the sheaf condition for sheaves on sites is defined as a Prop. I don't have a strong preference towards either design, but for the purpose of connecting the two theories, the design should be consistent on both sides.

That said, changing the sheaf condition to a Prop does seem to simplify things a little, as indicated by the net negative diff. The most changes are in unique_gluing, where some definitions have become no longer necessary.

Open in Gitpod

@justus-springer justus-springer added the awaiting-CI The author would like to see what CI has to say before doing more work. label Oct 7, 2021
@github-actions github-actions bot removed the awaiting-CI The author would like to see what CI has to say before doing more work. label Oct 7, 2021
@justus-springer justus-springer added the awaiting-review The author would like community review of the PR label Oct 8, 2021
@jcommelin jcommelin requested a review from b-mehta October 8, 2021 06:53
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.

I think the net size of the diff is a convincing argument for this PR. But I haven't done much with sheaves yet myself (besides reviewing). So I would like to hear from @b-mehta and/or @semorrison as well.

@@ -111,7 +113,7 @@ PresheafedSpace.ext _ _ (Spec.Top_map_comp f g) $ nat_trans.ext _ _ $ funext $
begin
dsimp,
erw [Top.presheaf.pushforward.comp_inv_app, ← category.assoc, category.comp_id,
(structure_sheaf T).presheaf.map_id, category.comp_id, comap_comp],
(structure_sheaf T).1.map_id, category.comp_id, comap_comp],
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason for this change? I find it less readable.
Idem in several other places.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for bringing this up. sheaf C X is now a subtype of presheaf C X, hence the projection sheaf.presheaf no longer exists. I was considering adding it back in as an alias for subtype.val, but then you'd have to change quite a few rws to erws, for example when applying naturality of a sheaf homomorphism. This is essentially because the type of sheaf homomorphisms F ⟶ G is now defined as F.val ⟶ G.val, not F.presheaf ⟶ G.presheaf.

I guess you could make sheaf.presheaf an abbreviation for subtype.val, but this felt a little weird to me as the former is longer than the latter.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I haven't read this carefully yet, but I'm also concerned here.

  1. I don't think .presheaf being longer than .1 is a problem; it's a good thing!
  2. What is the motivation for changing sheaf to a subtype, anyway? You lose the meaningful names, and gain ... what?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You gain consistency in the library: Sheaves on sites are already defined as a subtype: docs#category_theory.Sheaf. As I wrote above, I don't have a preference towards either design, but for the purpose of connecting the theories, I need consistency between them.

Personally, I think the mental jump between a sheaf and a presheaf isn't that high, so I'd say it's fine writing .1 or .val everywhere. I see that opinions can differ on that. All I can say is that #9609 would be a lot uglier if the design on both sides is not consistent.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As far as I can see, it all depends on whether you want the sheaf condition to be a Prop or a Type. If you want it to be a Prop, then defining sheaf as a subtype is the most natural choice. Then I don't really see the point of introducing the alias sheaf.presheaf, as it's just an extra layer of definition that needs to be unfolded and that makes everything longer. And I don't think writing F.val or F.1 instead of F.presheaf is that bad, it's done all the time for sheaves on sites. Of course, the alternative would be to make the sheaf condition a Type everywhere, but then the library of sheaves on sites had to be changed. And there are some formulations of the sheaf condition (e.g. unique gluings, amalgamations) which are clearly stated easier as a Prop instead of a Type.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay, I will accept the consistency argument. :-) (That said, maybe the sheaves on sites development should have used a structure with named fields rather than relying on subtype, but that question need not be addressed today!)

@semorrison
Copy link
Collaborator

I've marked as approved, but will wait a moment to hit merge in case @b-mehta or @jcommelin have further comments.

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

@github-actions github-actions bot 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 Oct 11, 2021
bors bot pushed a commit that referenced this pull request Oct 11, 2021
Make `sheaf_condition` into a `Prop` and redefine the type of sheaves on a topological space `X` as a subtype of `(opens X)ᵒᵖ ⥤ C`.
@bors
Copy link

bors bot commented Oct 11, 2021

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title refactor(topology/sheaves/*): Make sheaf condition a Prop [Merged by Bors] - refactor(topology/sheaves/*): Make sheaf condition a Prop Oct 11, 2021
@bors bors bot closed this Oct 11, 2021
@bors bors bot deleted the sheaf_condition_Prop_refactor branch October 11, 2021 10:24
@b-mehta
Copy link
Collaborator

b-mehta commented Oct 11, 2021

Apologies for not taking a closer look at this - I'm on board with making the sheaf condition a Prop: the question of whether Sheaf should be a subtype or structure is one I'm ambivalent on, so I have no objections to this PR!

@eric-wieser eric-wieser added the hacktoberfest-accepted Without this label hacktoberfest is scared off by bors label Oct 12, 2021
@alreadydone
Copy link
Collaborator

alreadydone commented Oct 16, 2021

The only changes I needed to make to an existing file is adding .some when extracting the is_limit condition; just a slight inconvenience!

@alreadydone
Copy link
Collaborator

Some comments have not been edited accordingly and still refers to names involving sheaf_condition instead of is_sheaf and equiv instead of iff, for example

`sheaf_condition_equiv_sheaf_condition_opens_le_cover`).

I've noticed this elsewhere in mathlib so am not sure if it's a priority to fix.

@justus-springer
Copy link
Collaborator Author

@alreadydone thanks for noticing! It seems like I forgot to change a few names in the comments. I fixed it here: #9807

bors bot pushed a commit that referenced this pull request Oct 19, 2021
As noted by @alreadydone in #9607, I forgot propagate naming changes to docstrings. This PR fixes that.
ericrbg pushed a commit that referenced this pull request Nov 9, 2021
As noted by @alreadydone in #9607, I forgot propagate naming changes to docstrings. This PR fixes that.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted Without this label hacktoberfest is scared off by bors 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

6 participants