-
Notifications
You must be signed in to change notification settings - Fork 139
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
Representable Presheaves, Free Category, Functor and Associated solvers #988
Conversation
ff09921
to
7d512f5
Compare
I'm converting this to draft as I have developed a lot more code along the same lines that should be useful to bundle together. |
Updated the description above. Ready for review now |
f93e07a
to
edd1020
Compare
I'll start reviewing... |
Thanks for all this stuff, this looks really useful! |
We don't have a strict 80-line policy, but we try to avoid very long lines. So use your own judgement, but I think in the following files, some lines are too long: Cubical/Categories/Constructions/Free/Category/Base.agda |
Cubical/Tactics/Reflection.agda Do you think I could also use that to refactor the solver for CommRings/Monoids/...? |
I think so. Even better if it were in agda-base as it's modified 1lab code already. |
I'm working on the column overflows (sorry). But I did push one actual (minor) change to the Free category implementation that I had already needed downstream. |
Good - thanks! |
Ok column overflows should be good now. |
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.
In general, the PR looks good and contains a lot of very useful stuff!
One important thing is, that we document what was taken from the 1lab (they use a different license and we should credit them properly in any case).
Otherwise, I mostly commented on things on the level of style questions and asked for more documentation.
|
||
{- Utilities common to different reflection solvers. | ||
|
||
Most of these are copied/adapted from the 1Lab |
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 there more code from the 1lab? I think it should be marked because it has a GPL like license
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.
Just this file is from the 1lab. I can add a link to the file but let me know if there's a specific format I should follow.
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.
Yes, a link is good. And maybe "1Lab (AGPL 3.0)" to make it clear, they use a different license. I'm afraid I don't actually know how to do things like this properly.
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.
It would probably be good to follow the SPDX format for this, as it is quite standard now. You can have a look at https://spdx.dev/ids/ for how to do so!
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.
IANAL but looking at this closer, GPL is more restrictive than MIT and you can't license GPL code under MIT. I think cubical would have to also include the AGPL license, and be distributed under a dual license.
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 is now tracked as #1018. The solution there includes adding spdx identifiers, so please do that where necessary.
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.
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 absolutely no issues with having this file be BSD-3 either
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.
Same (not that I see any code I've written 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 everyone for resolving this!
So we use "BSD-3-Clause" for this file.
I've already mentioned this to Felix and Max, but I think the definition of I have a simple PoC with these new definitions, but it's not fully formed yet (and I don't have anything regarding hetereogeneous Yoneda). |
84761fe
to
a3bdc90
Compare
Ok I've addressed the last changes with my most recent commits:
For the last point, the equivalence between terminal element and universal element would be simpler if the morphisms in the category of elements were defined differently as @jpoiret mentioned, but a non-trivial amount of code would have to be rewritten around the new definition and I didn't want to increase the scope of this PR any more so I didn't do that. |
So, I think the last missing bit is marking the 1lab file as BSD-3 instead of AGPLv3. Could you also remove the commented code in |
Fixed the license and removed the accidentally included comments in the latest force push. |
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.
Should be good to merge now.
This PR defines representable presheaves a formalization of the notion of a universal property. This includes
record
description of a universal morphism of a presheaf. See chapter 2 of Riehl's Category Theory in Context for a reference.