instance Arbitrary (Ratio a) is broken in general case and sometimes throws exceptions

[functora]

Instance Arbitrary (Ratio a) is broken in general case and sometimes throws exceptions. For example for Ratio Natural type will cause underflow exceptions during the tests. https://github.com/nick8325/quickcheck/blob/cb240f06e538ff8dfc01cb8439f1478d2f258511/src/Test/QuickCheck/Arbitrary.hs#L498-L500 I'm not sure it could be fixed by some constraints or not, maybe if not - it's not the best idea to have generic instance itself, and better to split it into multiple, depends on a parameter. Maybe we could have something like this (not sure, didn't tested it):

uFractional :: (Fractional a) => Gen a
uFractional =
  sized $ \n -> do
    denom <- chooseInt (1, max 1 n)
    numer <- chooseInt (0, n * denom)
    pure $ fromIntegral numer / fromIntegral denom

[nick8325]

Hmm, that's pretty annoying!

In the case of Ratio a, maybe it can be fixed by not using chooseInt, but instead using the Arbitrary instance for a. E.g. instead of denom <- chooseInt (1, max 1 n), something like denom <- resize (max 1 n) (arbitrary :: Gen a) `suchThat` (>= 1).

[MaximilianAlgehed]

What about implementing arbitrarySizedPositiveFractional and require Arbitrary a on Arbitrary (Ratio a) and doing something like:

instance (Integral a, Arbitrary a) => Arbitrary (Ratio a) where
  arbitrary = do
    r <- arbitrarySizedPositiveFractional
    s <- signum <$> arbitrary
    pure $ s % 1 * r
  shrink = shrinkRealFrac

[phadej]

The

instance a ~ Integer => Arbitrary (Ratio a) where ...

instance should be fine too.

The Ratio a is bad abstraction. It's kind of works with Natural but not really well (though this is Num issue), it kind of works with Int but not really (especially with small integers like Int8 or Int16). It's not great.

[MaximilianAlgehed]

I don't actually see the problem for Natural or Int. For Int8 and friends you run into some under and overflow issues but for practical purposes that shouldn't be an issue for Int.

[phadej]

but for practical purposes that shouldn't be an issue for Int.

https://gitlab.haskell.org/ghc/ghc/-/issues/21646

It's quite easy to overflow, even for 64bit Int; denominators can grow surprisingly fast.

OTOH, maybe QuickCheck is a tool to figure out where a logic using Ratio Int starts to not resemble proper rationals anymore.

[phadej]

Another, less strict version of instance a ~ Integer => Arbitrary (Ratio a) where ... is

class ArbitraryRatio a where
    arbitraryRatio :: Gen (Ratio a)

instance ArbitraryRatio a => Arbitrary (Ratio a) where arbitrary = arbitraryRatio

instance ArbitraryRatio Integer
instance ArbitraryRatio Natural
instance ArbitraryRatio Int8
instance ArbitraryRatio Word8
...
instance ArbitraryRatio Int
instance ArbitraryRatio Word

Some of these implementation maybe same, but I don't think it's possible to figure out "good" general instance, a clearly better one than (%) <$> arbitrary <*> arbitraryNotZero (IMO that instance is very good for fixed width integral types)

[MaximilianAlgehed]

OTOH, maybe QuickCheck is a tool to figure out where a logic using Ratio Int starts to not resemble proper rationals anymore.

Yes. For sure.

[MaximilianAlgehed]

I still don't see any argument against the signum instance I proposed?

[phadej]

I still don't see any argument against the signum instance I proposed?

I don't feel confident it covers all the cases of Ratio Int8, which an instance should (as that type is very small).

Also for something like Integer, generating one just to get its sign is not "cheap". (And that is common problem in QuickCheck, e.g. Arbitrary Word64 instance is very slow: when testing some bitfiddling code, the generation of Word64s is often taking more time than evaluating the property).

[MaximilianAlgehed]

Good point! It doesn't cover -1/128 - that's something to think carefully about. Although, we don't generate extremes in QuickCheck anyway. Another thing to think about.

You could special case the instance on the expensive cases (I.e. Integer and Natural) with some OverlappingInstances.