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

Refactor properties of lifts of families out of 26-descent #988

Merged
merged 18 commits into from Jan 27, 2024

Conversation

VojtechStep
Copy link
Collaborator

@VojtechStep VojtechStep commented Dec 19, 2023

The file consisted of two sections: proof that the pullback property of pushouts implies the dependent pullback property of pushouts (with some infrastructure for lifts of families of elements), and characterization of families over pushouts via descent data for pushouts.

The plan is to refactor the first part into more appropriate locations throughout the library.

@EgbertRijke
Copy link
Collaborator

Since these checks pass it is probably best if we merge this PR, because I’m getting into the territory of this PR with my spans pr

@VojtechStep
Copy link
Collaborator Author

I think it's a good idea to split this PR in two, one for the dependent-pullback-property<->pullback-property refactor, and one for the descent data refactor.

Unfortunately I still have to port and add some prose for the first part, and I don't know when I'll get back to working on this, so I guess feel free to wreak havoc on 26-descent, since in the end it will be deleted.

I'm okay with holding this PR and resolving conflicts after the spans PR is merged.

@EgbertRijke
Copy link
Collaborator

I’m working through all these files right now because the spans PR makes such fundamental changes to how pushouts work

@EgbertRijke
Copy link
Collaborator

My PR is changing every line of code in 26-descent. This probably means that there is no sensible way to resolve merge conflicts other than doing the work again that you have already been doing. So I'm going to try to do the work that you have been doing in this PR in my spans PR, and give 26-descent a thorough refactor. My apologies for the complications.

@EgbertRijke
Copy link
Collaborator

I’m not sure it is wise to keep working on this pull request. It would be better to tell me what you want to change so that I can do it in spans

@VojtechStep
Copy link
Collaborator Author

I looked at Spans and it doesn't look like you changed the parts of 26-descent that deal with lifts (understandably, since they don't interact with spans directly), so I thought I'd restrict this PR to moving that into more appropriate places in the library, and add some prose in the process so that I understand it better (my main motivation is that I want to be able to properly explain the intuition behind pullback-property -> dependent-pullback-property, since I don't really "get it").

AFAICT the intersection of this PR and Spans is that I just moved pullback-property-dependent-pullback-property-pushout, cone-family-dependent-pullback-property, is-pullback-cone-family-dependent-pullback-family and dependent-pullback-property-pullback-property-pushout to synthetic-homotopy-theory.dependent-pullback-property-pushouts, so would it be possible to keep this open and merge it before Spans, and then do the span thing in that file?

@EgbertRijke
Copy link
Collaborator

Yep that’s definitely possible. I’ll limit my work on 26-decent for the time being. Thanks for the heads up

@VojtechStep VojtechStep changed the title Refactor 26-descent Refactor properties of lifts of families out of 26-descent Jan 6, 2024
@VojtechStep VojtechStep force-pushed the refactor/descent-pushouts branch 2 times, most recently from 25536f3 to 28512f3 Compare January 12, 2024 21:04
@VojtechStep VojtechStep force-pushed the refactor/descent-pushouts branch 2 times, most recently from af1f4ee to a624aee Compare January 20, 2024 18:22
@VojtechStep VojtechStep marked this pull request as ready for review January 20, 2024 18:24
@VojtechStep
Copy link
Collaborator Author

This one is ready for review

@VojtechStep
Copy link
Collaborator Author

I added a table on the different conditions equivalent to "being a pushout", but I didn't include it in more places, because I wanted to reduce the amount of conflicts with #885

Copy link
Collaborator

@EgbertRijke EgbertRijke left a comment

Choose a reason for hiding this comment

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

Well done! That was a very nontrivial thing to refactor.

@EgbertRijke
Copy link
Collaborator

Since @VojtechStep has exams and this PR is crucial for our current refactoring efforts of synthetic homotopy theory, I'm thinking about merging this PR without the review comments being addressed. In that case I would address the review comments myself in #885. Would this be an acceptable way to proceed?

@fredrik-bakke
Copy link
Collaborator

fredrik-bakke commented Jan 27, 2024

Since @VojtechStep has exams and this PR is crucial for our current refactoring efforts of synthetic homotopy theory, I'm thinking about merging this PR without the review comments being addressed. In that case I would address the review comments myself in #885. Would this be an acceptable way to proceed?

Sure, no problem! Alternatively, if you want to be more tidy about it you can push to his branch before merging it.

@VojtechStep
Copy link
Collaborator Author

I didn't get to it yesterday, but I should be able to address the comments this afternoon

@EgbertRijke
Copy link
Collaborator

Since @VojtechStep has exams and this PR is crucial for our current refactoring efforts of synthetic homotopy theory, I'm thinking about merging this PR without the review comments being addressed. In that case I would address the review comments myself in #885. Would this be an acceptable way to proceed?

Sure, no problem! Alternatively, if you want to be more tidy about it you can push to his branch before merging it.

I never figured out how it works to push into someone else's branch. I always get prompted with an error about permission to do so.

@EgbertRijke
Copy link
Collaborator

I didn't get to it yesterday, but I should be able to address the comments this afternoon

If you want to do it then I can wait, but I don't want to request changes from you during your exams.

(Note that I already resolved the merge conflict for you, so you will need to pull before making changes.)

@VojtechStep
Copy link
Collaborator Author

Alright, the comments should be addressed now

@EgbertRijke EgbertRijke merged commit 711e76c into UniMath:master Jan 27, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants