Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Update bifunctors #1886
Update bifunctors #1886
Changes from 4 commits
63e3847
783f5b9
a95509d
1046b48
62b8d2b
720075a
56d82e7
2c9fce6
d78a876
e19d81e
f5e4d86
192a786
beed14d
f930232
da5cb5a
c41d397
2ae47ce
1426fd9
b662a72
f902579
c2616da
07b6ff4
2ed1065
6c975c1
31562d0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having to introduce various "buildequiv" constructions is a little annoying. One thing that would help would be to use
pose
orset
to put these in the context. This also has the advantage of a smaller proof term, since they will be shared. This applies to other parts of the recent changes as well.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm having trouble figuring out what exactly to
pose
in this case. Maybe I'm misinterpreting your suggestion, but I haven't been able to make things cleaner usingpose
here so far.I also have a tangential question: when we talk about wanting smaller proof terms, is the goal more readable terms, terms that are easier for coq to handle, or both?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example, you can add
pose (feq' := Build_DCatEquiv f' (fe':=fe')).
at the start of the proof, since that term is used twice in the proof. Same for other repeated terms.In this case, it makes the tactic script easier to read, and also should produce a proof term that is easier for Coq to handle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see. I think my confusion was really around the second question in this case, so thank you for clarifying.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you look at Coq's proof term when the
pose
command is used, it starts withlet feq' := Build_DCatEquiv f' in ...
, which shows that this has been factored out. It turns out thatfeq'
appears 40 more times in the proof term (with implicit arguments shown), so this is a non-trivial savings. (BTW, this proof term is around 4000 lines long!)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. It's very unintuitive to me that this would improve coq's performance, so I greatly appreciate the suggestions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment and the one below also raise two thorny issues with displayed categories I'd like to improve on:
exact isd0gpd.
everywhere). I would imagine there are more tricks like the above that can be used to help coq along here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm just skimming this, but I'm seeing lots of proofs getting longer. Can you explain in general what the latest changes are doing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea was to introduce
compose_catie'
(dcompose_catie'
), which is a variant ofcompose_catie
that I think will make it slightly easier to prove when a specific map is an equivalence. The proof is the same, but slightly longer because we need to insert all theBuild_CatEquiv
terms.This makes
compose_catie
a one-liner, but it also made the proof ofcate_inv_compose
longer as now there are twoBuild_CatEquiv
terms to deal with there. Because of this,dcate_inv_compose
also had to get longer.I thought
compose_catie'
would be useful in the future (as well as for provingBifunctor.iemap11
), but if it's not strictly better we should revert it. I have also needed it before e.g. to prove that 2-of-6 holds for equivalences.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see why factoring
compose_catie'
out ofcompose_catie
should make subsequent things harder to prove. I suspect that if you provecompose_catie'
nicely, it should reduce to the same proof ofcompose_catie
that you had before. But maybe the issue is that if you start with an equivalence, and then build a new equivalence with the same underlying map and same proof that it is an equivalence, you don't get back the equivalence you had? Unfortunately, I'm very busy for the next few days, so I won't have time to look into this, but I encourage you to see if there is a way to make this work out more simply. One obvious alternative is to revert to the old proof ofcompose_catie
, and then definecompose_catie'
using it, instead of the other way around.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will have a look. I suppose I wasn't sure which way around was preferable, but I think it's all resolvable with something like a
cate_homotopic
lemma.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have used
cate_homotopic
to redefinecompose_catie'
in terms ofcompose_catie
as you suggested. It would also be straightforward to remove one or both of these definitions if it's just simpler to avoid touching equivalences in this PR.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, that looks reasonable. I thought about this a bit more, and my first intuition was that your previous approach would be better, since normally we first prove
IsEquiv
and then use that to proveEquiv
. But with wild categories, it's trickier, since just from theCatIsEquiv
type, you don't have access to any data, such as the inverse. You first have to build the element ofCatEquiv
, and even then, that gives you a retraction and section for a potentially different map (which is homotopic to the one you started with, but not definitionally equal). So this makes working withCatIsEquiv
assumptions awkward. We could (1) throw up our hands and avoid makingCatIsEquiv
assumptions, instead havingCatEquiv
assumptions. (2) Add some lemmas that given a mapf
and a proof ofCatIsEquiv f
, return the inverseg
and the proofs thatf
itself is a section and a retraction ofg
. (This is an idiom that you are using in a few places.) (3) Think hard about the definition ofHasEquivs
and see if there is a way to make this work better. There's a lot of redundancy inHasEquivs
, and it's there to try to accommodate different settings, so it might not be possible to do better.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is (2) something worth a small refactor to make
CatIsEquiv
assumptions a better default? I guess maybe best to spend some time on (3) first.Do you know if anyone has tried to factor
CatEquiv
out ofHasEquivs
? I'm thinking of whether it's possible to provide the simplicity ofa $<~> b
without directly including it inHasEquivs
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(3) is definitely something that would take some thought and experimenting, not as part of this PR. I don't know what people have tried. Some "obvious" things, like just getting rid of
CatEquiv
as data, and instead defining it to be{ e : a $-> b & CatIsEquiv e }
won't work because we want to be able to use wild categories is situations where there is already an existing notion of equivalence. I'm not sure if that's what you meant by factoringCatEquiv out of
HasEquivs`. If you have another idea, please share it.In the meantime, something like (2) could be part of this PR, since I think it would just mean taking a few repeated idioms and factor them out into lemmas. But it could also be left as something to think about later, and this PR could stay like it is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am happy with leaving this to think on and potentially include in a separate PR for now (assuming others feel the same).