-
Notifications
You must be signed in to change notification settings - Fork 247
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
[Add] Consequences of associativity for Semigroup
s
#2688
base: master
Are you sure you want to change the base?
Conversation
For others: the names are taken from the combinators in |
With an explicit callback/reference to #2288 ? This PR alone doesn't 'fix' that issue, but contributes a piece of the jigsaw to such a 'fix'. |
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.
Lots of comments, I'm requesting changes based on a single one, but can be summarised as:
- please give lemmas names that can be remembered without needing a lookup table or other instruction manual to explain what they are/how they behave!
- no need for a new module, we already have the
Algebra.Properties.*
sub-hierarchy...
src/Algebra/Reasoning/Semigroup.agda
Outdated
module Pulls (ab≡c : a ∙ b ≈ c) where | ||
pullʳ : ∀ {x} → (x ∙ a) ∙ b ≈ x ∙ c |
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.
Elsewhere, the library would use lemma names of the form p∧q⇒r
, with square brackets for grouping sub-terms, so here:
module Pulls (x∙y≈z : x ∙ y ≈ z) where
x∙y≈z⇒[w∙x]∙y≈w∙z : (w ∙ x) ∙ y ≈ w ∙ z
NB. also: you have declared x
as a variable
, so there's no need to have the quantifier! Although here again, we have the problem that in any deployment in a concrete Semigroup
the typechecker might not be able to infer w
... so there are questions about how the quantifications should be handled.
If you can find examples in eg Data.Rational.Properties
which might be simplified by these lemmas, then you might discover whether Agda can (or not) infer the various implicits?
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.
Also, the use of ⇒
in the lemma names would avoid the need for the submodules here to be named; they could (more) simply be anonymous module _ (hyps) where
...
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.
While x∙y≈z⇒[w∙x]∙y≈w∙z
is a logical name, it is also quite long. As these combinators that re-associate are book-keeping things, I'd like to find something shorter / less noisy.
I do hate push/pull. I'm partial to on-right
.
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.
Well, with my suggested notational optimisations, this would/could become...xy≈z⇒wx∙y≈wz
, which is about as short as I can make 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.
As for on-right
I'm really not sure what that is supposed to signify, given that the lemma is not cong
like, nor is the action obviously happening on the right? (Never mind our ongoing debates about left/right distinctions giving rise to confusion...)
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.
on-right
as in apply this equality on the right
, after a re-focus. It's very much cong
-like to me, it just has a re-association done first, to put "the right" in focus.
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'd tried to reply, but my comment (seems to have) got lost: if you're wedded to an ASCII/prefix-Lisp name for these things, then I'd much prefer refocus-on-right
, or even refocus-right
(the -on
isn't then doing much work), or even refocusʳ
... in the style of other lemmas in the library.
Co-authored-by: jamesmckinna <[email protected]>
src/Algebra/Reasoning/Semigroup.agda
Outdated
module Algebra.Reasoning.Semigroup {o ℓ} (S : Semigroup o ℓ) where | ||
|
||
open Semigroup S | ||
using (Carrier; _∙_; _≈_; setoid; trans ; refl; sym; assoc; ∙-cong) |
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.
Indentation is by 2 spaces, not 4 as you have here... see style-guide
.
using (Carrier; _∙_; _≈_; setoid; trans ; refl; sym; assoc; ∙-cong) | |
using (Carrier; _∙_; _≈_; setoid; trans ; refl; sym; assoc; ∙-cong; ∙-congˡ; ∙-congʳ) |
src/Algebra/Reasoning/Semigroup.agda
Outdated
pushʳ {x = x} = begin | ||
x ∙ c ≈⟨ sym (∙-cong refl ab≡c) ⟩ | ||
x ∙ (a ∙ b) ≈⟨ sym (assoc x a b) ⟩ | ||
(x ∙ 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.
Successive appeals to sym
suggest that it should be lifted out (permutation of sym
through proofs), but in fact doing so quickly reveals the following:
pushʳ {x = x} = begin | |
x ∙ c ≈⟨ sym (∙-cong refl ab≡c) ⟩ | |
x ∙ (a ∙ b) ≈⟨ sym (assoc x a b) ⟩ | |
(x ∙ a) ∙ b ∎ | |
pushʳ = sym (pullʳ ab≡c) |
OK... thanks for the recent commits, but I'm going to carry on nitpicking:
|
Also:
As with previous comments, we don't need a new module (even if it's moved location), these could all go in |
|
||
module Assoc4 {u v w x : Carrier} where | ||
[[u∙v]∙w]∙x≈u∙[[v∙w]∙x] : ((u ∙ v) ∙ w) ∙ x ≈ u ∙ ((v ∙ w) ∙ x) | ||
[[u∙v]∙w]∙x≈u∙[[v∙w]∙x] = trans (∙-congʳ (assoc u v w)) (assoc u (v ∙ w) x) |
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 strong reason to have push/pull defined before these is that they can be used here. You should indeed move those up above Assoc4
and refactor the proofs here to use push/pull.
The 'new' names in |
Again, agree to disagree. No 'clear' failure on my side, but happy to try to find a good outcome to the naming discussion. |
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.
Placeholder review: this will have to wait until at least the weekend before I can return to it.
But I think in the meantime there still seem to be spacing issues, the existing question of where these things should live, as well as those regarding whether explicitly-named proofs of 'symmetric' versions are really 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.
I agree with a lot of what @jamesmckinna is saying.
- This doesn't need it's own module, i.e.
Algebra.Properties.Semigroup
is fine. - I much prefer the current set of names.
CHANGELOG.md
Outdated
@@ -123,6 +123,8 @@ New modules | |||
|
|||
* `Data.Sign.Show` to show a sign | |||
|
|||
* `AlgebraPropreties.Semigroup.Reasoning` adding reasoning combinators for semigroups |
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.
Lots of typos in the name?
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.
And indeed, if we do as I have suggested several times, and not make this a new module, but under additions to existing modules
, then it should be under Algebra.Properties.Semigroup
...
CHANGELOG.md
Outdated
@@ -123,6 +123,8 @@ New modules | |||
|
|||
* `Data.Sign.Show` to show a sign | |||
|
|||
* `AlgebraPropreties.Semigroup.Reasoning` adding reasoning combinators for semigroups | |||
|
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 also not sure whether we should refer to these as reasoning combinators
. The Reasoning
terminology has quite a specific use in the library for syntax that chains together nicely. I'm not sure these lemmas qualify?
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.
We won't need CHANGELOG
text if it moves to Additions to...
, but the opening comment block should be rephrased...
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.
Lots of comments and suggestion, but mostly a do-over of things I have already commented on before, and were not (yet) changed. TL;DR:
- move all the lemmas to
Algebra.Properties.Semigroup
- anonymous modules
- fix implicit/explicit quantification
It would be better for you to do (all) this, but if you are having problems with it, it's easy enough for me to take a copy of your branch and push the various changes to it...
CHANGELOG.md
Outdated
@@ -123,6 +123,8 @@ New modules | |||
|
|||
* `Data.Sign.Show` to show a sign | |||
|
|||
* `AlgebraPropreties.Semigroup.Reasoning` adding reasoning combinators for semigroups |
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.
And indeed, if we do as I have suggested several times, and not make this a new module, but under additions to existing modules
, then it should be under Algebra.Properties.Semigroup
...
CHANGELOG.md
Outdated
@@ -123,6 +123,8 @@ New modules | |||
|
|||
* `Data.Sign.Show` to show a sign | |||
|
|||
* `AlgebraPropreties.Semigroup.Reasoning` adding reasoning combinators for semigroups | |||
|
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.
We won't need CHANGELOG
text if it moves to Additions to...
, but the opening comment block should be rephrased...
Semigroup
s
Following @MatthewDaggitt 's comment about 'reasoning combinators', I've taken the liberty of renaming the PR to hopefully better reflect the intention of the contribution(s) here. |
Co-authored-by: jamesmckinna <[email protected]>
I moved everything into Semigroups and made the variables explicit. I tried to address all the comments, but I probably missed some. Sorry for not taking these changes into account earlier |
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.
Last round of changes, I hope, because I've looked at this to many times now... law of diminishing returns...
NB if you adopt my suggestions, there will no doubt be knock-on consequences (wrt parametrisation) which you should fix before committing... for the sake of any other reviewers!
UPDATED: and you still have a 4-space indentation for all the definitions, instead of the mandated 2... PLEASE FIX, along with the CHANGELOG
formatting, which has now broken.
@@ -123,7 +123,7 @@ New modules | |||
|
|||
* `Data.Sign.Show` to show a sign | |||
|
|||
Additions to existing modules | |||
* `Algebra.Properties.Semigroup` adding consequences for associtvity for semigroups |
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.
Typo
* `Algebra.Properties.Semigroup` adding consequences for associtvity for semigroups | |
* `Algebra.Properties.Semigroup` adding consequences for associativity for semigroups |
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.
And, in case it wasn't clear before: you do need to list, explicitly, every single lemma as an addition to that module (follow the model of the entries below), so this line should also move downwards to a separate block between Algebra.Properties.Monoid.Divisibility
and Algebra.Properties.Semigroup.Divisibility
...
... AND you've somehow deleted the line
Additions to existing modules
at L126... which you need to restore, sigh.
uv≈w⇒[xu∙v]y≈x∙wy x y = trans (∙-congʳ (uv≈w⇒xu∙v≈xw x)) (assoc _ _ _) | ||
|
||
uv≈w⇒[xy∙u]v≈x∙yw : ∀ x y → ((x ∙ y) ∙ u) ∙ v ≈ x ∙ (y ∙ w) | ||
uv≈w⇒[xy∙u]v≈x∙yw x y = trans (∙-congʳ (assoc x y u)) (uv≈w⇒[x∙yu]v≈x∙yw x y ) |
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.
whitespace
uv≈w⇒[xy∙u]v≈x∙yw x y = trans (∙-congʳ (assoc x y u)) (uv≈w⇒[x∙yu]v≈x∙yw x y ) | |
uv≈w⇒[xy∙u]v≈x∙yw x y = trans (∙-congʳ (assoc x y u)) (uv≈w⇒[x∙yu]v≈x∙yw x y) |
uv≈w⇒xu∙vy≈x∙wy : (x ∙ u) ∙ (v ∙ y) ≈ x ∙ (w ∙ y) | ||
uv≈w⇒xu∙vy≈x∙wy = uv≈w⇒xu∙v≈xw (uv≈w⇒u∙vx≈wx uv≈w _) _ |
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 think that this needs, for the usual reasons about the unifier/typechecker
uv≈w⇒xu∙vy≈x∙wy : (x ∙ u) ∙ (v ∙ y) ≈ x ∙ (w ∙ y) | |
uv≈w⇒xu∙vy≈x∙wy = uv≈w⇒xu∙v≈xw (uv≈w⇒u∙vx≈wx uv≈w _) _ | |
uv≈w⇒xu∙vy≈x∙wy : ∀ x y → (x ∙ u) ∙ (v ∙ y) ≈ x ∙ (w ∙ y) | |
uv≈w⇒xu∙vy≈x∙wy x y = uv≈w⇒xu∙v≈xw (uv≈w⇒u∙vx≈wx uv≈w x) y |
uv≈w⇒x∙wy≈x∙[u∙vy] : x ∙ (w ∙ y) ≈ x ∙ (u ∙ (v ∙ y)) | ||
uv≈w⇒x∙wy≈x∙[u∙vy] = sym (uv≈w⇒x[uv∙y]≈x∙wy uv≈w _ _) |
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.
And here
uv≈w⇒x∙wy≈x∙[u∙vy] : x ∙ (w ∙ y) ≈ x ∙ (u ∙ (v ∙ y)) | |
uv≈w⇒x∙wy≈x∙[u∙vy] = sym (uv≈w⇒x[uv∙y]≈x∙wy uv≈w _ _) | |
uv≈w⇒x∙wy≈x∙[u∙vy] : ∀ x y → x ∙ (w ∙ y) ≈ x ∙ (u ∙ (v ∙ y)) | |
uv≈w⇒x∙wy≈x∙[u∙vy] x y = sym (uv≈w⇒x[uv∙y]≈x∙wy uv≈w x y) |
uv≈w⇒xy≈z⇒u[vx∙y]≈wz : ∀ z → x ∙ y ≈ z → u ∙ ((v ∙ x) ∙ y) ≈ w ∙ z | ||
uv≈w⇒xy≈z⇒u[vx∙y]≈wz z xy≈z = trans (∙-congˡ (uv≈w⇒xu∙v≈xw xy≈z v)) (uv≈w⇒u∙vx≈wx uv≈w z) |
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.
And here:
uv≈w⇒xy≈z⇒u[vx∙y]≈wz : ∀ z → x ∙ y ≈ z → u ∙ ((v ∙ x) ∙ y) ≈ w ∙ z | |
uv≈w⇒xy≈z⇒u[vx∙y]≈wz z xy≈z = trans (∙-congˡ (uv≈w⇒xu∙v≈xw xy≈z v)) (uv≈w⇒u∙vx≈wx uv≈w z) | |
uv≈w⇒xy≈z⇒u[vx∙y]≈wz : ∀ x y → x ∙ y ≈ z → u ∙ ((v ∙ x) ∙ y) ≈ w ∙ z | |
uv≈w⇒xy≈z⇒u[vx∙y]≈wz x y xy≈z = trans (∙-congˡ (uv≈w⇒xu∙v≈xw xy≈z v)) (uv≈w⇒u∙vx≈wx uv≈w _) |
uv≈w⇒x∙wy≈x∙[u∙vy] : x ∙ (w ∙ y) ≈ x ∙ (u ∙ (v ∙ y)) | ||
uv≈w⇒x∙wy≈x∙[u∙vy] = sym (uv≈w⇒x[uv∙y]≈x∙wy uv≈w _ _) | ||
|
||
module _ {u v w x : Carrier} where |
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.
These will need to be explicit... for the usual reasons, but there's no need to make Carrier
explicit:
module _ {u v w x : Carrier} where | |
module _ u v w x where |
u[v∙wx]≈[u∙vw]x : u ∙ (v ∙ (w ∙ x)) ≈ (u ∙ (v ∙ w)) ∙ x | ||
u[v∙wx]≈[u∙vw]x = sym [u∙vw]x≈u[v∙wx] | ||
|
||
module _ {u v w x : Carrier} (uv≈wx : u ∙ v ≈ w ∙ x) where |
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.
Finally, here:
module _ {u v w x : Carrier} (uv≈wx : u ∙ v ≈ w ∙ x) where | |
module _ (uv≈wx : u ∙ v ≈ w ∙ x) where |
because the variable
declarations take care of the initial implicit prefix...
Introduce a new module for equational reasoning in semigroups, providing utilities for associativity reasoning and operations.