search

morganthomas / purescript-group

Algebraic groups for PureScript.
Apache License 2.0
6 stars 6 forks source link

Add Multiplicative instances #4

Closed MonoidMusician closed 6 years ago

MonoidMusician commented 7 years ago

I was going through the hierarchies and noticed that DivisionRing should be able to provide ginverse for Multiplicative. (I think this is correct – we shouldn't need a full (commutative) field, but a field should furnish a commutative group I think.)

Also I relaxed commutativeAdditive to only need a Semiring. Addition is always commutative in the hierarchy so might as well require just the base class :)

hdgarrood commented 7 years ago

This doesn't quite work because of the zero element - I think we'd need some other NonZeroMultiplicative newtype with a constructor which ensures that the thing it contains is not zero.

matthewleon commented 6 years ago

Only groupMultiplicative doesn't work here... The Commutative does, no? And the relaxing of commutativeAdditive makes sense as well, to me.

hdgarrood commented 6 years ago

Yep, I think the commutativeAdditive and commutativeMultiplicative instances are good.

morganthomas commented 6 years ago

I don't see any issues with this. For me it looks good to merge. Anybody disagree?

matthewleon commented 6 years ago

Looks right to me.

morganthomas commented 6 years ago

Thanks for the contribution @MonoidMusician !

MonoidMusician commented 6 years ago

Yes, thanks for the feedback from all of you! I think NonZeroMultiplicative is a good idea, I’ll remember to submit a PR for that later.

morganthomas commented 6 years ago

NonZeroMultiplicative = the multiplicative group of the non-zero elements of a division ring?

hdgarrood commented 6 years ago

Yep, exactly. On reflection I think NonZero might be a better name, as the Group instance using multiplication is kind of obvious, or at least the only remaining sensible option; you no longer have the zero element so it couldn’t possibly be using addition.