NonNegative Arbitrary instance generates a lot of 0s

[dcoutts]

For some reason the Arbitrary instance for NonNegative was chosen to produce 0 a lot, much more than e.g. Int.

Indeed there is a comment in the code questioning it:

instance (Num a, Ord a, Arbitrary a) => Arbitrary (NonNegative a) where
  arbitrary =
    (frequency
       -- why is this distrbution like this?
       [ (5, (NonNegative . abs) `fmap` arbitrary)
       , (1, return (NonNegative 0))
       ]
    ) `suchThat` ge0
    where ge0 (NonNegative x) = x >= 0

Since this modifier is the natural one to use to pick sizes of other structures then the frequency of 0 produces a lot of trivial test cases for other generators.

  arbitrary = do
    NonNegative size <- arbitrary
    ...

Can we change the probability of 0 for NonNegative so it is similar to that for a generator like Int?

How about using the approach for Positive:

instance (Num a, Ord a, Arbitrary a) => Arbitrary (NonNegative a) where
  arbitrary =
    ((Positive . abs) `fmap` arbitrary) `suchThat` ge0
    where ge0 (Positive x) = x >= 0

BTW, why does Positive use both suchThat (/= 0) and suchThat gt0? Seems redundant.

[dcoutts]

Sorry, too much copy'n'paste in that suggestion. Try again:

instance (Num a, Ord a, Arbitrary a) => Arbitrary (NonNegative a) where
  arbitrary =
    ((NonNegative . abs) `fmap` arbitrary) `suchThat` ge0
    where ge0 (NonNegative x) = x >= 0

[phadej]

I'd guess that NonNegative . abs <$> arbitrary alone vs. with explicit zero case is to guarantee zero case generation.

I think that explicit zero case is not necessary. E.g. even for double:

λ Test.QuickCheck> sample (arbitrary :: Gen Double)
0.0
0.9890583784693938
-2.3944265150420008
1.0381469551613405
2.3916795433670663
-2.6895946440038636
-6.556568708876091
-12.63625841132435
-3.10135404553042
-7.258718585154548
4.8306528328602125

though everything else is random-ish, first value is always 0.0 as it's generated with size = 0:

-- | Generates some example values.
sample' :: Gen a -> IO [a]
sample' g =
  generate (sequence [ resize n g | n <- [0,2..20] ])

[phadej]

Duncan: isn't fmap abs or suchThat (>=0) enough alone. Do we need both?

Sent from my iPhone

On 7 Oct 2018, at 20.34, Duncan Coutts notifications@github.com wrote:

Sorry, too much copy'n'paste in that suggestion. Try again:

instance (Num a, Ord a, Arbitrary a) => Arbitrary (NonNegative a) where arbitrary = ((NonNegative . abs) fmap arbitrary) suchThat ge0 where ge0 (NonNegative x) = x >= 0 — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

[nick8325]

Thanks for the patch, which I've applied!

I asked @koengit, who wrote the comment, and he wasn't sure why the instance was like this. The only reason I can think of is that without it you won't generate 0 :: Double unless the size is 0, but this seems like something that should be fixed in the Arbitrary Double instance if anywhere.

The reason why we need suchThat (>= 0) is because abs can return a negative number: abs (minBound :: Int) = minBound. It would be enough to just use suchThat and not abs, but using abs may be faster as it avoids the need to retry generation in the common case.