Generate Rational numbers with smaller denominators

[Bodigrim]

Closes #295, making maximal denominator dependent on size instead of being fixed to 9999999999999.

> sample (arbitrarySizedFractional :: Gen Rational)
0 % 1
(-1) % 1
3 % 4
4 % 3
(-1) % 1
8 % 5
1 % 1
(-3) % 2
4 % 7
(-2) % 3
(-1) % 1

[phadej]

This doesn't generate Rational larger han 1 does it?

[phadej]

Oh it does, but it's harder to see what kind of distribution we get. Can't we just limit the precision to max 256 size or something like that, in the original code?

[Bodigrim]

Well, sometimes you may want to generate numerators bigger than 256. I have such applications, for instance.

The comment does not guarantee anything about distribution beyond "The number can be positive or negative and its maximum absolute value depends on the size parameter", which still holds. Even the range of distribution remains unchanged.

This implementation is also consistent with smallcheck instance for Ratio.

[phadej]

With

 arbitrarySizedFractional =
   sized $ \n ->
     let n' = toInteger n in
-      do b <- chooseInteger (1, precision)
+      do b <- chooseInteger (1, max 256 n')
          a <- chooseInteger ((-n') * b, n' * b)
          return (fromRational (a % b))
- where
-  precision = 9999999999999 :: Integer

one gets

0 % 1
(-7) % 39
505 % 129
(-349) % 98
825 % 124
1599 % 242
(-1187) % 246
(-219) % 80
(-414) % 31
1293 % 97
1103 % 158

--

Yet, I'd against this change alone (especially to fix #295). The Double and Float ought to have own hand written instances. @cartazio's idea to have half like above and half using full Double / Float precision is good. There are plenty of numerical bugs to be discovered by IEEE rounding issues e.g. generating (only) coarse numbers will make some users unhappy as well.

[Bodigrim]

What IEEE rounding issues do you have in mind? The proposed implementation is perfectly capable to show that associativity does not hold for Double, for example:

> quickCheck $ \a b c -> a + (b + c) === ((a + b) + c :: Double)
*** Failed! Falsified (after 12 tests and 6 shrinks):
-0.2
1.0
-0.1
0.7 /= 0.7000000000000001

[Bodigrim]

I do not mind Arbitrary Double to be not just arbitrary = arbitrarySizedFractional, but rather oneof [arbitrarySizedFractional, somethingElse], but deciding on somethingElse is out of scope of this particular PR, which aims to improve arbitrarySizedFractional first of all.

Your approach with max 256 n' excludes (almost all) short ratios from being generated. I would disagree that this is beneficial for any Fractional. This is undesirable even for Double: e. g., your generator is unsuitable for testing rounding half to even, because it almost never generates numbers of form ddd.5.

[phadej]

Your distribution looks like

So I don't see how it would generate ddd.5 numbers either often enough. Yet, true, we should generate such numbers.

[Bodigrim]

Err, I'm not sure what facts about half-integers can be derived from this plot (and what distribution you desire to obtain). As quoted above, sample generates at least some half-integers, on contrary to the existing implementation and to your suggestion.

[cartazio]

Andrew: what do you think of the hybrid mixture in Olegs alternative Pr? I think both have some good bits

…

On Sat, May 16, 2020 at 3:17 PM Bodigrim ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In Test/QuickCheck/Arbitrary.hs <#296 (comment)>: > @@ -998,13 +998,10 @@ inBounds fi g = fmap fi (g `suchThat` (\x -> toInteger x == toInteger (fi x))) -- and its maximum absolute value depends on the size parameter. arbitrarySizedFractional :: Fractional a => Gen a arbitrarySizedFractional = - sized $ \n -> - let n' = toInteger n in - do b <- chooseInteger (1, precision) - a <- chooseInteger ((-n') * b, n' * b) - return (fromRational (a % b)) - where - precision = 9999999999999 :: Integer + sized $ \n -> do + numer <- chooseInt (-n, n) + denom <- chooseInt (1, max 1 n) Err, I'm not sure what facts about half-integers can be derived from this plot (and what distribution you desire to obtain). As quoted above, sample generates at least some half-integers, on contrary to the existing implementation and to your suggestion. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#296 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAABBQQ3EBGLEESIHZTLSDTRR3RDXANCNFSM4NC6TXMA> .

[Bodigrim]

As discussed above, @phadej and I are tackling different aspects of the issue. However, after some consideration, I came to a conclusion that it is futile to improve arbitrarySizedFractional: it does not make much sense for Fractionals other than Double or Rational. E. g., it returns unbounded by size results for modular arithmetic or Galois fields. And given that #297 does a very good job for Double, the only thing left is Rational. That said, I decided to repurpose this PR to improving instance Arbitrary (Ratio a).

[Bodigrim]

@nick8325 just a gentle reminder about this PR.

[Bodigrim]

@nick8325 ping

[Bodigrim]

@nick8325 ping

[nick8325]

Sorry about the unreasonably-long delay, but it's merged now. Thanks for the patch!

I tweaked the generator to do this:

  sized $ \n -> do
    denom <- chooseInt (1, max 1 n)
    numer <- chooseInt (-n*denom, n*denom)
    pure $ fromIntegral numer / fromIntegral denom

so that the generated numbers are drawn roughly uniformly from the interval -n...n, and have a maximum denominator of n. That is, increasing the size increases both the magnitude and the maximum denominator. I hope that's reasonable but let me know if you see some problem with it.

[Bodigrim]

@nick8325 that's probably fine, thanks.