-
Notifications
You must be signed in to change notification settings - Fork 705
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
WIP:adding alegebraic typeclasses #1551
base: series/8.0.x
Are you sure you want to change the base?
Conversation
The failing tests are because the |
Does Group have any laws? I don't know if we need it... (non rhetorical, curious to see what people think). cc @TomasMikula @jdegoes ? Edit: s/Group/Magma |
You mean does magma have any laws? It doesn't. I'm not terribly interested
in having it.
…On Dec 1, 2017 7:20 PM, "Vincent Marquez" ***@***.***> wrote:
Does Group have any laws? I don't know if we need it... (non rhetorical,
curious to see what people think). cc @TomasMikula
<https://github.com/tomasmikula> @jdegoes <https://github.com/jdegoes> ?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1551 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAadHwideFmmDoS5n9rwtgHOcwwGwUdnks5s8ENSgaJpZM4QyliC>
.
|
|
||
/* A Group is a Monoid with negation and inverse, such that | ||
* a |+| inverse(a) = monoid.empty | ||
* inverse(a) |+| a = monoid.empty |
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.
Aren't these Group
laws? @vmarquez
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 are the laws, as per @jdegoes comment #1526 (comment) we will implement laws later on
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.
Yeah. I wrote this after not realizing that "Does Group have any laws?" was intended to mention "Magma" instead.
|
||
trait GroupInstances { | ||
|
||
implicit val int: Group[Int] = new GroupClass[Int] { |
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.
This implies that Int
's (semi)group/monoid instance is (0, +), not (1, *).
Is that intended?
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 question, I have no answer for it, except that perhaps it is not time yet to add instances
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 sure could use some tagging that @S11001001 worked on in Scalaz 7.
* */ | ||
trait Magma[A] { | ||
def append(a1: A, a2: => A): 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.
Surely this is lawless. What good does it do us to have a Magma
that's not a Semigroup
?
@@ -1,6 +1,6 @@ | |||
package scalaz | |||
package typeclass | |||
|
|||
trait SemigroupClass[A] extends Semigroup[A]{ | |||
trait SemigroupClass[A] extends Semigroup[A] with MagmaClass[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.
These subclassings need to be reflected in BaseHierarchy
.
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.
even if we consider removing Magma, I added it to BH as well
3162600
Yeah, i'm of the opinion that if an algebraic structure doesn't have laws, let's ditch it. |
I completely agree that Magma seems unnecessary, and it adds awkwardness to the code... |
the only "law" that Magma must enforce is that the result of the binary operation belongs to the same set... which I would think is already given by the type system, will remove it. |
the only "law" that Magma must enforce is that the result of the binary operation belongs to the same set... which I would think is already given by the type system
@caente is this a WIP? If so, can you label it/change the name? When you're ready for a review/merge, can you squash the commits too? thanks :) |
@vmarquez I was hopping to have smaller PRs, but this is the first time doing this kind of things, and I'm not used to the slow pace... name changed, and definitely will squash commits. |
I'm actually waiting for some feedback regarding Rings @vmarquez @jdegoes @hrhino @TomasMikula |
also @jdegoes mentioned Commutative as a typeclass, but it seems to me that it should "just" be a law? I'd still want to understand why are we deferring laws implementation? Is it lack of consensus? |
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.
Looking better.
I think that adding tut
examples (see the examples/
directory) would help to ensure that all instances are properly resolved, and (of course) provide useful documentation for these types.
@NeQuissimus has done some great work in other scalaz 8 PRs regarding the examples; you should check them out.
|
||
trait AbelianGroupInstances { | ||
|
||
implicit val int: AbelianGroup[Int] = new AbelianGroupClass[Int] { |
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 if you have this here it's probably going to clash with GroupInstances#int
if you ask for Group[Int]
. You should (assuming we intend to provide a Group[Int]
at all) keep this and remove the other.
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.
yeah, I agree with your above comment, these instances belong to examples, will move them
implicit def groupMonoid[A](implicit A:Group[A]):Monoid[A] = A.monoid | ||
implicit def abelianGroupGroup[A](implicit A:AbelianGroup[A]):Group[A] = A.group | ||
implicit def ringAbelianGroup[A](implicit A:Ring[A]):AbelianGroup[A] = A.abelianGroup | ||
implicit def ringMonoid[A](implicit A:Ring[A]):Monoid[A] = A.monoid |
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.
This worries me; a Ring
contains two distinct Monoid
s, and this picks the "multiplicative" one. However, groupMonoid compose abelianGroupGroup compose ringAbelianGroup
also produces a Monoid
from a Ring
. This will cause the somewhat counter-intuitive behavior where asking for Monoid[A]
gives you the multiplicative one, but calling a method that asks for Group[A]
and then internally uses Monoid[A]
will find the additive one.
I think we should have a rule such as
newtype Mult a = Mult a
instance Ring a => Monoid (Mult a) where
...
using the (as yet forthcoming) tag types.
@@ -1,6 +1,11 @@ | |||
package scalaz | |||
package typeclass | |||
|
|||
/** | |||
* A Semigroup is a binary opertion such that: | |||
* 1 - The result is in the same Set/Type |
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.
This seems a little extraneous, as a quick glance at the type of append
tells you exactly this. I'd opt for terseness and say
A semigroup is a type equipped with an associative binary operation
Also, opertion
is misspelled...
I think the lack of laws has been due to a lack of bandwidth. I was under the impression that scalaprops had been decided on as the law checking framework. I'd like to go poke at that some this week, if I find time. |
@caente We don't have machinery to encode laws just yet. We need a data structure such as: final case class IsEqual[A](lhs: A, rhs: A) Then all the "laws" can return data. It's OK to have a type class that merely adds another law to an existing structure (e.g. I will review in more depth later. |
I'm going to try to revive this tomorrow at the scalaz summit. |
First stab at expanding the Algebra hierarchy.
EDIT: Removed Magma