-
Notifications
You must be signed in to change notification settings - Fork 257
[ add ] Pointwise lifting of algebra to Data.Vec.Functional (Functional vector module #1945 redux) #2817
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: master
Are you sure you want to change the base?
Conversation
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 few minor style things, but this looks quite good.
lift₁ᴹ f = map f | ||
|
||
-- Vector RawMagma construction | ||
VecRawMagma : RawMagma a ℓ → (n : ℕ) → RawMagma a ℓ |
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 use (say) Data.Sum.Algebra
as a guide for naming conventions of these.
|
||
-- Left scalar action obtained from the ring/semiring multiplication | ||
_*ₗ_ : {S : Set a} → Op₂ S → Opₗ S (Vector S n) | ||
_*ₗ_ _*_ r xs = map (λ x → _*_ r x) xs |
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.
_*ₗ_ _*_ r xs = map (λ x → _*_ r x) xs | |
_*ₗ_ _*_ r xs = map (r *_) xs |
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, worth adding some global variable
declarations to avoid having to mention {S : Set a}
prefixes in types at all?
|
||
-- Right scalar action (same underlying Op₂, but as a right action) | ||
_*ᵣ_ : {S : Set a} → Op₂ S → Opᵣ S (Vector S n) | ||
_*ᵣ_ _*_ xs r = map (λ x → _*_ x r) xs |
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.
_*ᵣ_ _*_ xs r = map (λ x → _*_ x r) xs | |
_*ᵣ_ _*_ xs r = map (_* r) xs |
isSemiringVec = record { | ||
isSemiringWithoutAnnihilatingZero = record | ||
{ +-isCommutativeMonoid = isCommutativeMonoid n +-isCommutativeMonoid | ||
; *-cong = λ x≈y u≈v i → *-cong (x≈y i) (u≈v i) |
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 are many combinators in Function.Base
that could be used here.
No review as such, but an observation about a global problem with both
Some of this seems to come about because of name clashes, either directly, or as a result of local Suggest also that, for all these update/'redux' PRs:
|
pointwiseᵛ : ( _≈_ : Rel A ℓ ) → Rel (Vector A n) ℓ | ||
pointwiseᵛ _≈_ = Pointwise _≈_ |
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.
Is this (redundant) repetition worth the gain (eg in uniformity of naming?)
I'd be tempted to do two things here:
- make the
import
on L20 qualified,as Pointwise
- inline all uses of
pointwiseᵛ
in favour simply ofPointwise
as it stands...
zipWithᵛ : Op₂ A → Op₂ (Vector A n) | ||
zipWithᵛ _∙_ = zipWith _∙_ | ||
|
||
mapᵛ : Op₁ A → Op₁ (Vector A n) | ||
mapᵛ f = map f |
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. (and all the more so, now that 3 imported definitions are being subject to the same treatment...)
rawMagmaᵛ : RawMagma a ℓ → (n : ℕ) → RawMagma a ℓ | ||
rawMagmaᵛ M n = |
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.
Now that M is being referenced only locally via the where
clause, again... I'd drop the superscript.
_*ₗᵛ_ : {S : Set a} → Op₂ S → Opₗ S (Vector S n) | ||
_*ₗᵛ_ _*_ r xs = map (r *_) xs |
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.
Two suggestions:
- eta-contract (no need to mention
xs
on either LHS or RHS) - now you've used
map
, rather thanmapᵛ
, its alias...
... so I'd go all the way now:
_*ₗᵛ_ : {S : Set a} → Op₂ S → Opₗ S (Vector S n) | |
_*ₗᵛ_ _*_ r xs = map (r *_) xs | |
_*ₗ_ : {S : Set a} → Op₂ S → Opₗ S (Vector S n) | |
_*ₗ_ _*_ r = map (r *_) |
_*ᵣᵛ_ : {S : Set a} → Op₂ S → Opᵣ S (Vector S n) | ||
_*ᵣᵛ_ _*_ xs r = map (_* r) xs |
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.
module VM = RawMagma magmaᵛ | ||
module M = RawMagma rawMagma | ||
|
||
open IsEquivalence |
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.
Is this needed?
isMagmaᵛ : IsMagma M._≈_ M._∙_ → IsMagma VM._≈_ VM._∙_ | ||
isMagmaᵛ base = record | ||
{ isEquivalence = flip Pointwise.isEquivalence _ B.isEquivalence | ||
; ∙-cong = λ x≈y u≈v i → B.∙-cong (x≈y i) (u≈v i) | ||
} | ||
where module B = IsMagma base |
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.
Now you've chosen base
for the 'base' argument, you won't need the superscript...?
As for module B = IsMagma base
why change the names module CM = ...
, module SG = ...
of the subsequent locally-bound modules... when B
would suffice in those instances as well?
monoidᵛ = rawMonoidᵛ rawMonoid n | ||
module VM = RawMonoid monoidᵛ | ||
module RM = RawMonoid rawMonoid |
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.
Again, I see no use of monoidᵛ
besides the definition of module VM
, so consider inlining it:
monoidᵛ = rawMonoidᵛ rawMonoid n | |
module VM = RawMonoid monoidᵛ | |
module RM = RawMonoid rawMonoid | |
module VM = RawMonoid (rawMonoid M n) | |
module M = RawMonoid M |
etc. No need for RM
here?
module' : Module _ _ _ | ||
module' = record { isModule = isModule } No newline at end of file |
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.
For the underlying name of the bundle, stdlib
uses in eg. Algebra.Module.Construct.TensorUnit
the name ⟨module⟩
, and I would follow suit here, for the sake of consistency.
Thanks for the updates @e-mniang ! My recent round of nitpicks really home in on wanting to avoid the superscripts altogether:
Hope this
|
FYI: @e-mniang says she'll finish this once she's established a proper rhythm with this term's courses, back in France. |
Safe journeys back! Given the current merge hiatus thanks to GitHub, it seems fine to also have a hiatus in fretting too much about when updates get pushed... ... while commentary remains the stream that can never be shut off! |
This PR updates #1945 by bringing the functional vector and module structures/bundles in line with current stdlib conventions and removing nested submodules.
Next, I’ll add the necessary entry to the CHANGELOG.
Reviews and comments are welcome.