-
Notifications
You must be signed in to change notification settings - Fork 260
Add ideals and quotient rings #2855
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
base: quotient-groups
Are you sure you want to change the base?
Conversation
jamesmckinna
left a comment
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.
All looks good. Lots of stylistic nitpicking though!
| record Ideal c′ ℓ′ : Set (c ⊔ ℓ ⊔ suc (c′ ⊔ ℓ′)) where | ||
| field | ||
| -- a (two sided) ideal is exactly a subbimodule of R considered as a bimodule over itself |
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 put the comment above the record declaration, and even embellish it to:
| record Ideal c′ ℓ′ : Set (c ⊔ ℓ ⊔ suc (c′ ⊔ ℓ′)) where | |
| field | |
| -- a (two sided) ideal is exactly a subbimodule of R considered as a bimodule over itself | |
| ------------------------------------------------------------------------ | |
| -- Definition | |
| -- a (two-sided) ideal is exactly a sub-bimodule of R considered as a bimodule over itself | |
| record Ideal c′ ℓ′ : Set (c ⊔ ℓ ⊔ suc (c′ ⊔ ℓ′)) where | |
| field |
| record Ideal c′ ℓ′ : Set (c ⊔ ℓ ⊔ suc (c′ ⊔ ℓ′)) where | ||
| field | ||
| -- a (two sided) ideal is exactly a subbimodule of R considered as a bimodule over itself | ||
| subbimodule : Subbimodule (TU.bimodule {R = R}) 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.
I personally find the parametrisation in TensorUnit to make this kind of thing more ugly to read than is wholly necessary (I'd perhaps downstream even reconsider some of the implicit vs. explicit quantification there), and, because Subbimodule already takes an {R : Ring _ _} parameter, I'd be tempted to write instead:
| subbimodule : Subbimodule (TU.bimodule {R = R}) c′ ℓ′ | |
| subbimodule : Subbimodule {R = R} (TU.bimodule) c′ ℓ′ |
which would then permit the earlier import of TU to actually be handled in place as
open import Algebra.Module.Construct.TensorUnit using (bimodule)I'd even be tempted to go further: conventionally, the canonical name for the tensor unit (wrt whatever tensor product structure) is \bI, so I'd even make this a renaming import
open import Algebra.Module.Construct.TensorUnit renaming (bimodule to 𝕀)but this is being hyper-nitpicky! (maybe!?)
| open import Algebra.Properties.Semiring semiring | ||
| open import Algebra.Properties.Ring R |
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.
A pity that we don't (yet) have it so that the second presupposes the first cf. #2804 ?
| open import Algebra.Structures | ||
| open import Function.Definitions using (Surjective) | ||
| open import Level | ||
| open import Relation.Binary.Reasoning.Setoid setoid |
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.
Ditto. should this be part of the API exported by Algebra.Properties.X? Do we ever invoke algebraic properties not in a context where we have access to equational reasoning wrt that algebra?
| open import Algebra.Properties.Semiring semiring | ||
| open import Algebra.Properties.Ring R | ||
| open import Algebra.Structures | ||
| open import Function.Definitions using (Surjective) |
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.
Unused, AFAICT?
| renaming (≋-∙-cong to ≋-+-cong; ≋-⁻¹-cong to ≋‿-‿cong) | ||
|
|
||
| open import Algebra.Definitions _≋_ | ||
| open import Algebra.Morphism.Structures using (IsRingHomomorphism) |
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 bumped up above the private block?
JacquesCarette
left a comment
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're a picky bunch... nice stuff!
| open import Level | ||
| open import Relation.Binary.Reasoning.Setoid setoid | ||
|
|
||
| ≋-*-cong : 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.
I would be tempted to use a local module (I forget the syntax for doing just inside the proof part) where I is opened so that all these qualifiers can go away. It's an idiom used a lot in 1lab that I need to integrate into my own Agda.
| } | ||
| ; *-cong = ≋-*-cong | ||
| ; *-assoc = λ x y z → ≈⇒≋ (*-assoc x y z) | ||
| ; *-identity = record |
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 this is 'just' a pair, use _,_ instead of a record? And maybe also eta contract?
| { fst = λ x → ≈⇒≋ (*-identityˡ x) | ||
| ; snd = λ x → ≈⇒≋ (*-identityʳ x) | ||
| } | ||
| ; distrib = record |
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.
_,_ would work here too (but not eta)
| private module I = Ideal I | ||
| open I using (ι; normalSubgroup) |
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.
You can, somewhat counterintuitively, simplify this to
| private module I = Ideal I | |
| open I using (ι; normalSubgroup) | |
| private open module I = Ideal I using (ι; normalSubgroup) |
| project-isHomomorphism : IsRingHomomorphism rawRing quotientRawRing project | ||
| project-isHomomorphism = record |
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.
| project-isHomomorphism : IsRingHomomorphism rawRing quotientRawRing project | |
| project-isHomomorphism = record | |
| project-isRingHomomorphism : IsRingHomomorphism rawRing quotientRawRing project | |
| project-isRingHomomorphism = record |
As with #2854 maybe have the algebra name X in the name, as well as the type?
| open I using (ι; normalSubgroup) | ||
|
|
||
| open import Algebra.Construct.Quotient.Group +-group normalSubgroup public | ||
| using (_≋_; _by_; ≋-refl; ≋-sym; ≋-trans; ≋-isEquivalence; ≈⇒≋; quotientIsGroup; quotientGroup; project; project-surjective) |
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.
How much of this list actually needs to be exported?
Yes, you use ≋-refl in particular, as well as ≈⇒≋ but the rest of the generic ≋-isEquivalence structure is not used, and could be recreated if needed from quotientGroup? (variations on the theme of #2391 )
| { Carrier = Carrier | ||
| ; _≈_ = _≋_ | ||
| ; _+_ = _+_ | ||
| ; _*_ = _*_ | ||
| ; -_ = -_ | ||
| ; 0# = 0# | ||
| ; 1# = 1# | ||
| } |
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.
| { Carrier = Carrier | |
| ; _≈_ = _≋_ | |
| ; _+_ = _+_ | |
| ; _*_ = _*_ | |
| ; -_ = -_ | |
| ; 0# = 0# | |
| ; 1# = 1# | |
| } | |
| { RawRing R.rawRing hiding (_≈_) | |
| ; _≈_ = _≋_ | |
| } |
avoids redundant DRY ...?
| { isGroup = quotientIsGroup | ||
| ; comm = λ x y → ≈⇒≋ (+-comm 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.
Don't need to create, nor import, quotientIsGroup here; can be recreated on the fly:
| { isGroup = quotientIsGroup | |
| ; comm = λ x y → ≈⇒≋ (+-comm x y) | |
| } | |
| { isGroup = isGroup | |
| ; comm = λ x y → ≈⇒≋ (+-comm x y) | |
| } where open Group quotientGroup using (isGroup) |
| quotientRing : Ring c (c ⊔ ℓ ⊔ c′) | ||
| quotientRing = record { isRing = quotientIsRing } |
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.
Similarly, can we inline the definition of quotientIsRing here?
|
In lieu of any further review comments, please see #2859 (purely!) as a comparison point, and feel free to grab anything from there which you might find useful... it just seemed the easiest way to encapsulate all the thoughts I had had about your groups-rings-modules development |
Add definition of ideals and quotient rings. Based on #2854. Taken from #2729