-
Notifications
You must be signed in to change notification settings - Fork 141
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
Additions to the Powerset module #1056
base: master
Are you sure you want to change the base?
Conversation
Could some reorganization be done to avoid duplication? Another thing is that I generally don't like operations that take hProp's as input and produce hProp's if they don't have to. It's often better to have something produce a Type and then have the proof that it is an h-prop separately. The reason being that many times one can do things purely for Type's without any need to pass around the h-prop proofs. A more technical issue is that Agda sometimes get confused and implicit arguments tend to work worse as Agda cannot infer the h-prop proofs... I made a comment about this at some point (https://github.com/agda/cubical/blob/master/Cubical/Functions/Logic.agda#L6) and would be quite happy to delete/rewrite the Logic file... Maybe this could be done as part of reorganization so that you can avoid duplication in this PR? |
The main aspect is that
I agree that |
I'm not against splitting Functions.Embeddings into multiple files in a separate directory and then have a file exporting everything if this helps with dependencies. I'm also all for rewriting Functions.Logic. Feel free to have a go at it in this PR! |
Sure, I'll have a go at it! :D |
Cubical/Foundations/Powerset.agda
Outdated
@@ -111,7 +111,7 @@ private data _⊎_ (A B : Type ℓ) : Type ℓ where | |||
inr : B → A ⊎ B |
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 think it's a good idea to have two versions of a datatype, especially since they are not definitionally equal. It would be better to split Cubical.Data.Sum into Cubical.Data.Sum.Base and .Properties, import sum from .Base, and have .Properties import from Embeddings so there is no circular dependency and no duplication. (And I don't think making it private fixes it: How does a user of this library give an element of A U B without being able to access the constructors of the datatype underlying it?)
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 think it's a good idea to have two versions of a datatype, especially since they are not definitionally equal. It would be better to split Cubical.Data.Sum into Cubical.Data.Sum.Base and .Properties, import sum from .Base, and have .Properties import from Embeddings so there is no circular dependency and no duplication. (And I don't think making it private fixes it: How does a user of this library give an element of A U B without being able to access the constructors of the datatype underlying it?)
Fixes #1052
Do note that two definitions had to be rewritten to prevent circular dependencies.
I've highlighted them in the comments and marked them as
private
.