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

Colimit library #850

Merged
merged 18 commits into from
Mar 8, 2017
Merged

Colimit library #850

merged 18 commits into from
Mar 8, 2017

Conversation

SimonBoulier
Copy link
Contributor

Hi everyone,
This PR add the colimit library as described in #845.
Please feel free to comment.
Here are some debatable points on which I could work if you think they prevent merging:

  • The tactic funext (I find it very convenient, but could be removed if you don't like it.)
  • The notation which is utf8
  • The lack of comments

The file CommutativeSquares.v and a part of the file Diagram.v are from the [hott-limits library](https://github.com/peterlefanulumsdaine/hott-limits/) by Jeremy Avigad, Krzysztof Kapulkin, Peter LeFanu Lumsdaine.
@mikeshulman
Copy link
Contributor

Looks like travis timed out?

I wouldn't block on funext or the lack of comments, but I still think we should keep the library ASCII as much as possible. Would you mind doing a search-and-replace?

@mikeshulman
Copy link
Contributor

Oh, and thanks!!

@mikeshulman
Copy link
Contributor

Thanks, LGTM once Travis can be fixed. I'm not sure what the problem is; could it be just that the library is now honestly taking too long to compile? Could you attach a timing report?

@SimonBoulier
Copy link
Contributor Author

Here it is: time-of-build-both.log
But it does not explain the 20 min overhead at all ...

Require Import Colimits.Diagram Colimits.Colimit.
Generalizable All Variables.

Context `{Funext}.
Copy link
Contributor

Choose a reason for hiding this comment

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

You should wrap this development in a section. What you are doing here is declaring a Funext axiom, which is not what we want.

Local Open Scope path_scope.
Generalizable All Variables.

Context `{Funext}.
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be inside a section, preferably after the record definition and the Arguments commands, which should stay at top-level

Definition path_cocone {C1 C2: cocone D X}
(eq1 : forall i, C1 i == C2 i)
(eq2 : forall i j g x, qq C1 i j g x @ eq1 i x = eq1 j (D _f g x) @ qq C2 i j g x)
: C1 = C2.
Copy link
Contributor

Choose a reason for hiding this comment

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

Style nit: I think we generally add Proof. after Definition and Lemma

Copy link
Member

Choose a reason for hiding this comment

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

Seconded.

Defined.

Definition postcompose_cocone (C: cocone D X) {Y: Type}
: (X -> Y) -> cocone D Y.
Copy link
Contributor

Choose a reason for hiding this comment

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

Also Proof. here

:= Build_cocone colim pp.

Lemma is_universal_colimit {G: graph} (D: diagram G)
: is_universal (cocone_colimit D).
Copy link
Contributor

Choose a reason for hiding this comment

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

Add Proof. after Lemma

(* We show here that the pushout defined as a colimit *)
(* is equivalent to the pushout defined as a primitive HIT. *)
Section is_PO_pushout.
Require Import Types.Sum HIT.Pushout.
Copy link
Contributor

Choose a reason for hiding this comment

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

Require inside Section is unsupported, I think

Defined.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: any reason for this whitespace change?

Defined.

Definition equiv_moveL_transport_V {A : Type} (P : A -> Type) {x y : A}
(p : x = y) (u : P x) (v : P y)
: transport P p u = v <~> u = transport P p^ v
:= BuildEquiv _ _ (moveL_transport_V P p u v) _.


Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: why the extra newline?

Defined.

Definition equiv_moveL_transport_p {A : Type} (P : A -> Type) {x y : A}
(p : y = x) (u : P x) (v : P y)
: transport P p^ u = v <~> u = transport P p v
:= BuildEquiv _ _ (moveL_transport_p P p u v) _.


Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Why the extra newline?

Proof.
destruct p, q; reflexivity.
Defined.
(* Remark: this is also equals to: *)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: "equal to" not "equals to"

@JasonGross
Copy link
Contributor

Regarding travis: the log suggests that the issue is proviola. Comparing the occurrences of "Travis keep-alive spew" to a normal build suggests that it's fine all the way up through running XSLTPROC proviola-xml/HoTT.HIT.Suspension.xml, and then something whacky happens where it doesn't finish when trying to run that command....

@@ -775,6 +775,22 @@ Ltac by_extensionality x :=
simpl; auto with path_hints
end.


Copy link
Member

Choose a reason for hiding this comment

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

These tactics should be commented so that people can use them even if they don't know how to write tactics themselves.


(* ***************** *)
(* ***** Coeq ****** *)
(* ***************** *)
Copy link
Member

Choose a reason for hiding this comment

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

What is the purpose of these comments? If you are trying to visually serparate parts of the development, please use the coqdoc sectioning mechanism so that the generated documentation will look reasonable.

Coercion q : cocone >-> Funclass.


(** *** Definition of colimits *** *)
Copy link
Member

Choose a reason for hiding this comment

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

Please use coqdoc sectioning instead. The documentation says: "Sections are introduced by 1 to 4 leading stars (i.e. at the beginning of the line) followed by a space. One star is a section, two stars a sub-section, etc. The section title is given on the remaining of the line." So this should be something like:

(** ** Definitions of colimits *)



(** *** Existence of colimits *** *)
(** The existence is given by an HIT. *)
Copy link
Member

Choose a reason for hiding this comment

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

See above, please use coqdoc sectioning.


Arguments colim {G D} i x.

Axiom pp : forall {G: graph} {D: diagram G} (i j: G) (f : G i j) (x: D i),
Copy link
Member

Choose a reason for hiding this comment

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

Is pp really the best possible name here?

intros i j h x; simpl; hott_simpl. apply ap_compose.
Defined.

(* precompose *)
Copy link
Member

Choose a reason for hiding this comment

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

I would like to request lots of good comments, but I'll already be happy with some good comments that help the reader understand what is what. This particular one is not of that kind :-)

f_ap. exact (eisretr (postcompose_cocone HQ1) _)^.
Defined.

(** Here we prove than two equivalent diagrams have equivalent colimits. *)
Copy link
Member

Choose a reason for hiding this comment

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

All we ever do in Coq files is proving, no need to emphasize that. So I would say simply "Two equivalent diagrams have equivalent colimits."

@andrejbauer
Copy link
Member

Many thanks for your contribution. I would suggest (see also my comments in the code):

  1. Use coqdoc sectioning to delimit parts of development, rather than ad-hoc visually pleasing comments.
  2. Always start proofs with Proof.
  3. Please, please, put in some comments that will help people understand what is in the files. Each section or file should have a short overview of its contents, and non-obvious parts of the code should have comments. If the structure of a proof is not obvious, point that out in a comment and explain what it is.
  4. Whatever @JasonGross says.

@spitters
Copy link
Member

I did not have the time to look at this very carefully.
However, generally, this looks very nice.
Thanks @SimonBoulier !

: sigma_diag (fun _ : A => D) ~d~ prod_diag.
Proof.
unshelve econstructor.
- serapply Build_diagram_map; cbn.
Copy link
Member

Choose a reason for hiding this comment

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

is `serapply supported already. I do not find it here:
https://coq.inria.fr/refman/tactic-index.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was added in the HoTT library: PR #829.
I don't think than it is planed that Coq have such a tactic. Do you have heard something about that?

@spitters
Copy link
Member

I had a quick look. It looks like you've made the requested changes. Thanks!

@SimonBoulier
Copy link
Contributor Author

Yes! I only hope there is not too many spelling errors in the comments ...

@spitters
Copy link
Member

spitters commented Feb 13, 2017 via email

@SimonBoulier
Copy link
Contributor Author

Regarding the time out bug. I can reproduce it on my computer: make -j4 proviola never ends and stay stuck on XSLTPROC proviola-xml/HoTT.HIT.Suspension.xml
And indeed, there is a problem with this file: the nested comment just before loop_susp_unit is badly handled and make the whole file corrupted. I propose #852.

@SimonBoulier
Copy link
Contributor Author

The problem for proviola was with make proviola-html/HoTT.HIT.Colimits.Flattening.html which never ends.
I don't understand why but changing the Lets in Definitions at the end of Flattening.v solve the problem (maybe the goals are too long?).
However there are still some errors with the end of this file: the goals generated by proviola are not exactly the same as those I see on Coq.

Anyway, it' ok for me for merging.

@andrejbauer
Copy link
Member

If I understand correctly, Travis is now happy? Excellent. Many thanks for the PR. That's one pair of eyes from me.

@spitters
Copy link
Member

I'll let @JasonGross pull this, as I am not sure of the proviola part. It looks good from my perspective.

Copy link
Contributor

@JasonGross JasonGross left a comment

Choose a reason for hiding this comment

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

I'll let Travis be the arbiter of proviola. LGTM modulo adding Proof and either picking more descriptive names where I mentioned, or making those things Local Definition. Thanks!


(** We define here the graph ∫D, also denoted G·D *)

Definition integral : graph.
Copy link
Contributor

Choose a reason for hiding this comment

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

Add Proof. after Definition


(** Given a dependent diagram, we can recover a diagram over G by considering the Σ types. *)

Definition sigma_diagram : diagram G.
Copy link
Contributor

Choose a reason for hiding this comment

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

Add Proof. after Definition


(** Now, given an equifibered diagram and using univalence, one can define a type family [E' : colimit D -> Type] by recusrion on the colimit. *)

Definition E' : colimit D -> Type.
Copy link
Contributor

Choose a reason for hiding this comment

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

Add Proof. after Definition

Copy link
Contributor

Choose a reason for hiding this comment

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

With such a short name as E', you should make this Local Definition so that the unqualified name never gets exported. (You may also want Local Definition for the other things that were Let s; that's up to you.)

Context (A0 : A -> Type) (B0 : B -> Type) (C0 : C -> Type)
(f0 : forall x, A0 x <~> B0 (f x)) (g0 : forall x, A0 x <~> C0 (g x)).

Definition P : PO f g -> Type.
Copy link
Contributor

Choose a reason for hiding this comment

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

Add Proof. after Definition

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, Local Definition here, or pick a more descriptive name.

etransitivity. symmetry. apply f0. apply g0.
Defined.

Definition E : dep_diagram (span f g).
Copy link
Contributor

Choose a reason for hiding this comment

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

Add Proof. after Definition, and make this Local Definition or pick a more descriptive name.

exact (fun y => p # (g0 x y)).
Defined.

Definition HE : equifibered _ E.
Copy link
Contributor

Choose a reason for hiding this comment

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

Add Proof. after Definition, and make this Local Definition , or pick a more descriptive name.

@SimonBoulier
Copy link
Contributor Author

Thanks for your attention @JasonGross, I missed this file (for the Proof.). And I didn't know about Local Definition, it' nice.

@SimonBoulier
Copy link
Contributor Author

Anything blocking?

@JasonGross JasonGross merged commit 6712d5e into HoTT:master Mar 8, 2017
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

5 participants