typelevel / algebra

Experimental project to lay out basic algebra type classes
https://typelevel.org/algebra/
Other
378 stars 69 forks source link

Simplify RingLaws: Don't pass around a Predicate, be more explicit about requiring non-zero elements. #111

Closed TomasMikula closed 6 years ago

TomasMikula commented 8 years ago

Hi,

I present this PR for discussion. The goal is to simplify RingLaws, namely the case when we need a generator of non-zero values. The approach is to be more explicit about what laws require non-zero elements. I added an extra Arbitrary[A] argument to group properties and "descendants", which then allow to pass a generator of non-zero values from multiplicativeGroup and multiplicativeCommutativeGroup. multiplicativeGroup and multiplicativeCommutativeGroup now also explicitly require an instance of AdditiveMonoid in order to access zero to create non-zero generator.

I find it slightly clearer than the predicate approach. Whether you agree is up for discussion, but by the sheer number of lines saved, it is a simplification.

non commented 8 years ago

Hi Tomas,

Sorry I didn't see this earlier. I need to have a closer look to how things are threaded through. I have two (possibly unfounded) concerns:

  1. I worry that we will accidentally use the non-zero arbitrary implicitly in a place where we want the full arbitrary and end up failing to test certain properties with zero.
  2. In general we have tried to move away from using .filter on Arbitrary[_] instances to avoid cases where ScalaCheck aborts due to throwing out too many test cases. Removing one element may be OK, but I think some generators end up creating tons of zeros. The usual fix is to use .map and replace zero with a non-zero value. In this generic context we could use one (since I think all the "non-zero" properties are involving rings).
TomasMikula commented 8 years ago

I removed the implicit instance of non-zero arbitrary. (Thanks @johnynek for the tip.)

johnynek commented 6 years ago

seems stale, so closing. Happy to revisit.