-
Notifications
You must be signed in to change notification settings - Fork 247
[ fix #1694 ] Add ordered algebraic structures. #1752
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
Conversation
(BTW, I also have an implementation of residuated pomonoids and lattices, but that will need some more refactoring before it's ready to become a PR.) |
e2dcf11
to
e0e5db4
Compare
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 for the PR! Looks pretty great, just a few comments. As I mentioned, I don't have much knowledge in this area of algebra so I'm trusting that the definitions are sensible. If anyone else has more knowledge, then another review would be welcome!
src/Algebra/Ordered/Structures.agda
Outdated
|
||
record IsPosemigroup (_∙_ : Op₂ A) : Set (a ⊔ ℓ₁ ⊔ ℓ₂) where | ||
field | ||
isPartialOrder : IsPartialOrder _≈_ _≤_ |
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.
Hmm the lack of symmetry between this and IsProsemigroup
is a bit off-putting. The latter uses the algebraic structure IsSemigroup
as the base structure, whereas this uses the ordered structure IsPartialOrder
as the base structure. Is there a good reason for this? If not I think it'd be better for them to be consistent!
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.
Yea, I see what you mean. The reason is that you get some structure for free once you assume anti-symmetry as well. Notably, you get ∙-cong
for free or really cong
for any operation that is also monotone (as you noted yourself). There's a tradeoff here between adding redundant fields/assumptions to the records vs. keeping the record structure "regular". I personally prefer not to add redundant fields unless there are good technical reasons to do so (e.g. because it makes some proofs simpler). But I don't feel strongly about this, so I'm happy to discuss alternatives.
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.
Yup, I see your point and this is exactly the reason we have Algebra.Structures.Biased
which provide alternate, "easier" ways of constructing structures when you can derive some of the axioms from the others. So I'd recommend that we preserve the symmetry here, and then add this variant to Algebra.Ordered.Structures.Biased
? (which is also re-exported from Algebra.Ordered
for easy use).
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.
Alright. Two questions:
- Which structure do you prefer: using the algebraic structure as a base and adding what's missing from the orders, or vice-versa? (We could also add both at once, but then
isEquivalence
would be duplicated, which I really don't think is a good idea.) - Is there maybe a better name than
Biased
for this? I don't really see in which sense either version is biased. But I also don't have any good alternative suggestions, TBH...
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.
Upon reflection, the answer to 1. is probably to go with preorders as the base since we're going to mirror the whole algebra hierarchy (starting with magmas) and build them up anyway. For the po* structures, I'll similarly start with partial orders.
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.
using IsPreorder directly is not a good idea because it would duplicate the IsEquivalence field which you really don't want, there shouldn't be two versions of that in the same record
Agreed!
Hmm so I just wrote 3 paragraphs about why it was better with the algebra hierarchy as a base, but deleted it as I can't distinguish in my head whether I think that because the current algebra hierarchy is so hopelessly broken or whether its actually better that I way. In the end, I've decided at this moment I also don't have the time to work through all the implications (sorry future me!). Let's just keep it the way you've got it 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.
Yea, I can totally see that there are good arguments for and against here. It's hard to know which will work out better in the end.
Out of curiosity: when you say the algebra hierarchy is broken, do you think there would be a reasonably clean fix, or is it just an impossible problem to design hierarchies like this in Agda? I mean, it does feel like we're just implementing OO/multiple inheritance manually here, but maybe my judgment is clouded from having worked in OO languages too much in the past. In any case, I'm curious to hear if you have any ideas on how things could be improved in general. Is there some discussion around this topic somewhere? E.g. in an issue?
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.
do you think there would be a reasonably clean fix, or is it just an impossible problem to design hierarchies like this in Agda?
I think the consensus is that it's the latter. There's no good way to define diamonds compositionally without biasing them in some regards that impacts usability. I hear that Arend's implementation of record types is supposed to have gone a long way to fixing the problem.
In no particular order here are a selection of the issues discussing problems:
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 for these pointers. I'll bookmark #1238, which contains a lot of references to relevant related work. (Thanks @JacquesCarette!) If it's not already the case, it might be nice to summarize these discussions in some sort of README.
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.
No problem. [Sorry for the late comment, I am just now catching up to a couple of months' worth of agda-stdlib activity!]
src/Relation/Binary/Lattice/Properties/BoundedJoinSemilattice.agda
Outdated
Show resolved
Hide resolved
src/Relation/Binary/Lattice/Properties/BoundedMeetSemilattice.agda
Outdated
Show resolved
Hide resolved
src/Algebra/Ordered/Definitions.agda
Outdated
open import Data.Product using (_×_) | ||
|
||
Monotonic₁ : Op₁ A → Set _ | ||
Monotonic₁ f = f Preserves _≤_ ⟶ _≤_ |
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.
Hmm I'm now wondering if these should go in Relation.Binary.Definitions
instead of 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.
The reason I put them here is that they involve binary/unary operations from Algebra
. There are analogous Congruence
definitions in Algebra.Definitions
:
agda-stdlib/src/Algebra/Definitions.agda
Lines 27 to 31 in e0e5db4
Congruent₁ : Op₁ A → Set _ | |
Congruent₁ f = f Preserves _≈_ ⟶ _≈_ | |
Congruent₂ : Op₂ A → Set _ | |
Congruent₂ ∙ = ∙ Preserves₂ _≈_ ⟶ _≈_ ⟶ _≈_ |
I figured that, if these live in the Algebra
hierarchy (rather than Relation.Binary
), then so should Monotonic
and Antitonic
.
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.
So actually _Preserves _⟶ _
and friends are from the Relation.Binary
hierarchy. I think the notion of monotonic and antitonic functions make sense for any functions, rather than just those that are used in an algebraic setting right? For example f x = x ::_
is monotonic over the lexicographic ordering over lists.
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.
That's true, but it's equally true for Congruent
. I mean the only difference between Preserves
, Monotone
and Congruent
is how they are intended to be used. Maybe adding all these anti/mono/whathever definitions is overkill and we should just use Preserves
directly? (That's what I did originally, but then I figured I should probably mirror the definition of Congruent
.)
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.
That's true about Congruent
. Perhaps having it in Algebra.Definitions
instead of Relation.Binary.Definitions
is a mistake. Nonetheless, I would really like these new monotonic definitions to live in the latter.
Another argument is that if we ever want to add a new ordered definition that used both the order and the equality relation, then this module's current module-level arguments wouldn't allow that.
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.
OK, so just to double check before making the changes:
- I'll move all the definitions from
Algebra.Definitions.Ordered
toRelation.Binary.Definitions
then. - I'll drop the use of
Op
fromAlgebra.Definitions
(to make the definition generic, and avoid cycles in the module imports). - I'll keep
{Mono,Andt}tonic
even though they will be essentially aliases ofPreserves
.
Agreed?
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.
Sounds good 👍
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.
OK, done.
There isn't a guide I'm afraid.
|
Thanks for the review @MatthewDaggitt. I'll update the PR to address the obvious stuff, but I think some points warrant further discussion. In particular, I'm not an algebraist either. I mechanized this stuff a while back because I needed it for my experiments with graded (co)monads. Maybe @andreasabel, @JacquesCarette or @Taneb, who are part of the discussion in #1694 have some additional comments and ideas? |
Oh, and @laMudri too! |
I'll take a look this evening! |
e0e5db4
to
4cbd95e
Compare
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 like to see some proofs in Data.Nat.Properties
giving examples of these structures, too, but that could be in a later PR
Adding example properties about Also: does anyone know if it's possible to create a PR based on another one without the changes from the first PR showing up in the first one? I like to split PRs into manageable chunks but it kinda defeats the point if everything just gets lumped together anyway... |
Happy to have them added to this one. They're conceptually part of the same addition to the library, and it's always a good sanity check to actually use things after they've been defined to check there aren't any unforeseen wrinkles.
Not easily. The only way I could think of for not having the changes in the 1st PR show up in the 2nd PR without having the merged the 1st one is making a PR against your original PR rather than the master branch. But I don't think you can do such a thing on this repo, only your own fork? |
4cbd95e
to
b3da48f
Compare
The updated PR now contains example properties about |
Apologies, I didn't realise your changes to |
By the way thank you for your patience with this @sstucki, I realise it's a lot of refactoring. The design decisions are complicated, and definitely best to try and get them right now. |
OK, I'll factor this into a separate PR then. |
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.
Looks good! In the long run, I think we need a better approach to generating these definitions, but this is probably the best we can do for now.
open IsPartialOrder isPartialOrder public | ||
|
||
∙-cong : Congruent₂ ∙ | ||
∙-cong = mono₂⇒cong₂ _≈_ _≈_ Eq.sym reflexive antisym mono |
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's always an argument with these derived properties that they should appear in an auxiliary constructor rather than in the main record. In this case, I'm not really sure; I think we often have a proof of ∙-cong
lying around anyway, but maybe we don't actually gain anything from putting it in because we never really have to look at these proof terms. This could come up if we want to take the opposite ordering and have that be a definitional involution, but maybe because mono
should stay the same, ∙-cong
does too.
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, I was thinking along the same lines. @MatthewDaggitt actually suggested a different design where the algebraic structures (rather than the orders) are used as a base, and then there's a better case for keeping cong
and adding the type of constructors you mention (see our discussion further up in the thread). I'd be curious to hear which design you'd prefer.
src/Algebra/Ordered/Structures.agda
Outdated
field | ||
isPreorder : IsPreorder _≈_ _≤_ | ||
∙-cong : Congruent₂ ∙ | ||
mono : Monotonic₂ ∙ |
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'd be nice to have monoˡ
and monoʳ
, too, like for 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.
Good point, will add those! 👍
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.
Uh, now I'm confused. The type of congˡ
is LeftCongruent
which actually states that the operation is congruent in its second, i.e. right argument. And similarly for RightCongruent
. Is that a bug? Or some special meaning of "left" and "right" that I'm missing?
agda-stdlib/src/Algebra/Definitions.agda
Lines 33 to 37 in 650e05f
LeftCongruent : Op₂ A → Set _ | |
LeftCongruent _∙_ = ∀ {x} → (x ∙_) Preserves _≈_ ⟶ _≈_ | |
RightCongruent : Op₂ A → Set _ | |
RightCongruent _∙_ = ∀ {x} → (_∙ x) Preserves _≈_ ⟶ _≈_ |
Also, I'm not a huge fan of left/right because it doesn't generalize to n-ary operations. How about mono₁
and mono₂
?
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.
Hmm, yeah, I'd have thought congˡ
had type ∀ {x x′ y} → x ≈ x′ → x ∙ y ≈ x′ ∙ y
too. I can justify the current naming as saying that left-multiplication (by x
) preserves equivalence, but when I'm using it in a proof, I don't think I'd think that way. IIRC, there's a similar problem with distributivity or zero, where one is the reverse of the other (something like LeftZero
means 0 * x ≈ x
but LeftDistributive
means x * (y + z) ≈ x * y + x * z
, or vice versa).
As for numbers, I guess it side-steps any existing naming conventions, but introduces the problem of needing to choose between being 0-based or 1-based. I think (3+n)-ary operations are very rare in algebra, so I'm not too worried about that potential generalisation.
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.
OK, I can see that someone might have had good reasons to name {Left,Right}Congruent
the way they did and I don't want to break backwards compatibility, so I'll not "fix" this unless @MatthewDaggitt tells me to. As for the mono
variants, I'll go with mono₁
and mono₂
instead of for monoˡ
and monoʳ
for now, mostly because I don't know whether to follow that strange backwards terminology or not. (Again, unless @MatthewDaggitt tells me otherwise.)
b3da48f
to
b24b76d
Compare
Well, thank you for your patience @MatthewDaggitt, and for all your hard work on the Agda stdlib and for the community in general — much appreciated! |
b24b76d
to
594f239
Compare
@MatthewDaggitt and @Taneb: the changes to |
594f239
to
272f0b2
Compare
272f0b2
to
db85aa3
Compare
I think I have now addressed all the open requests for changes. I hope I didn't miss anything, as there have been quite a few discussions going on in parallel. 😅 (Thanks to all involved!) |
@MatthewDaggitt, is there anything more you need me to do here? |
Sorry for the delay. Merging in! Thanks again for the great PR @sstucki! |
I should add that if you are planning to make the changes to |
Thanks for all the help @MatthewDaggitt!
I have already prepared the changes for the PR in a separate branch (just separated them out from what was in this PR earlier). I'm afraid, it does precisely what you ask me not to do. 😓 If it's OK with you, I'll submit the PR as is anyway, and if the readability/maintainability is indeed worse, I'll refactor it to keep as many of the old proofs as possible. Does that sound OK? |
This is a starting point for addressing #1694 and eventually #481 (since we need promonoids for grading).
I implemented this a while ago but never got around to submitting a PR so I refactored it to fit current stdlib conventions as best I could, but please let me know if anything looks wrong. The PR is also missing a CHANGELOG patch, but I need some advice on what to put where there. (Is there a CHANGELOG guide somewhere?)