Infinities as floating point range endpoints #68

curiousleo · 2020-06-25T09:52:35Z

Fixes #54. Brings the behaviour in line with the docs, which state that if exactly one endpoint is infinite, then uniformRM returns that endpoint.

curiousleo · 2020-06-25T09:55:35Z

src/System/Random/Internal.hs

@@ -806,6 +809,9 @@ uniformDoublePositive01M g = (+ d) <$> uniformDouble01M g
 instance UniformRange Float where
  uniformRM (l, h) g
    | l == h = return l
+    | isInfinite l && isInfinite h = return (0/0) -- NaN


This was previously the case too, tested here:

random/src/System/Random/Stateful.hs

Lines 608 to 611 in 11464aa

-- * If \(a\) is @-Infinity@ and \(b\) is @Infinity@, the result is @NaN@.

--

-- >>> let (a, b, x) = (-inf, inf, 0.5) in x * a + (1 - x) * b

-- NaN

But we need this guard now so we can distinguish "both endpoints are unequal infinities" (this case) from "one of the endpoints is an infinity" (the next two cases)..

curiousleo · 2020-06-25T09:57:13Z

test/Spec.hs

  => Proxy a -> TestTree
 floatingSpec px =
  testGroup ("(" ++ showsType px ")")
  [ SC.testProperty "uniformR" $ seeded $ Range.uniformRangeWithin px
+  , testCase "r = +inf, x = 0" $ positiveInf @?= fst (uniformR (0, positiveInf) (ConstGen 0))
+  , testCase "r = +inf, x = 1" $ positiveInf @?= fst (uniformR (0, positiveInf) (ConstGen 1))
+  , testCase "l = -inf, x = 0" $ negativeInf @?= fst (uniformR (negativeInf, 0) (ConstGen 0))


This test fails without the additional guards in uniformRM, and passes when they are added.

curiousleo · 2020-06-25T10:16:16Z

The benchmarks show an unacceptable decline in performance. I'll turn this back into a draft until I've figured this out.

Before (11464aa):

pure/random/Float                        mean 27.71 μs  ( +- 555.9 ns  )
pure/uniformR/unbounded/Float            mean 31.27 μs  ( +- 3.095 μs  )

Currently (ef2ee10):

pure/random/Float                        mean 15.57 ms  ( +- 2.282 ms  )
pure/uniformR/unbounded/Float            mean 15.89 ms  ( +- 2.239 ms  )

Edit: see #68 (comment) for updated benchmarks.

curiousleo · 2020-06-25T10:37:22Z

64c52a5 makes Float perform as before, but Double takes twice as long as the baseline ... (Edit: I've optimised the code so this is no longer the case.)

Before (master / 11464aa):

pure/random/Float                        mean 31.08 μs  ( +- 4.587 μs  )
pure/uniformR/unbounded/Float            mean 31.72 μs  ( +- 3.938 μs  )
pure/uniformR/floating/pure/uniformFloat01M mean 30.22 μs  ( +- 3.292 μs  )
pure/uniformR/floating/pure/uniformFloatPositive01M mean 28.35 μs  ( +- 1.074 μs  )

pure/random/Double                       mean 28.02 μs  ( +- 540.9 ns  )
pure/uniformR/unbounded/Double           mean 31.69 μs  ( +- 2.741 μs  )
pure/uniformR/floating/pure/uniformDouble01M mean 30.83 μs  ( +- 2.935 μs  )
pure/uniformR/floating/pure/uniformDoublePositive01M mean 28.66 μs  ( +- 1.471 μs  )

After 67e12bf:

pure/random/Float                        mean 28.02 μs  ( +- 853.7 ns  )
pure/uniformR/unbounded/Float            mean 31.87 μs  ( +- 3.505 μs  )
pure/uniformR/floating/pure/uniformFloat01M mean 27.99 μs  ( +- 1.198 μs  )
pure/uniformR/floating/pure/uniformFloatPositive01M mean 29.99 μs  ( +- 3.298 μs  )

pure/random/Double                       mean 28.22 μs  ( +- 795.3 ns  )
pure/uniformR/unbounded/Double           mean 31.90 μs  ( +- 4.201 μs  )
pure/uniformR/floating/pure/uniformDouble01M mean 31.09 μs  ( +- 3.051 μs  )
pure/uniformR/floating/pure/uniformDoublePositive01M mean 31.19 μs  ( +- 3.234 μs  )

curiousleo · 2020-06-25T11:18:41Z

Performance issues fixed.

src/System/Random.hs

Shimuuar · 2020-06-25T11:38:06Z

makes Float perform as before, but Double takes twice as long as the baseline ...

I've observed such behavior when I implemented uniformDouble01M changes such as adding/removing benchmark led to 2x performance jumps. It looks like GHC might run or skip some optimization

curiousleo · 2020-06-25T16:10:25Z

Ready for review.

idontgetoutmuch · 2020-06-26T05:50:53Z

test/Spec.hs

  -- TODO: Add more tests
  ]
+  where
+    positiveInf, negativeInf :: a
+    positiveInf = read "Infinity"


I would write positiveInf = 1 / 0 then you don't need the Read constraint but I don't know if this is good practice.

It is certainly much faster. Probably even constant-folded

Considering that it is in a test suite, I don't think performance matters much. Using read makes it a bit more obvious I think.

Bodigrim · 2020-07-01T20:03:20Z

I think this is ready to be merged. @Shimuuar @idontgetoutmuch if either of you has a strong opinion about changing read "Infinity", please voice it.

Shimuuar · 2020-07-03T22:45:48Z

Sorry, I forgot about this PR. Why NaN is returned when both endpoints are infinities? Following definition (in pseudocode) seems more sensible:

uniformRM (+∞,+∞) _ = return +∞
uniformRM (-∞,-∞) _ = return -∞
uniformRM (-∞,+∞) _ = oneOf [-∞,+∞]

Also number of ifs on happy path (neither endpoint is ∞) worry me a bit.

Bodigrim · 2020-07-03T22:56:28Z

Why NaN is returned when both endpoints are infinities?

This has been discussed back in #54. I agree with @idontgetoutmuch that returning NaN is more reasonable than alternation between infinities.

Bodigrim · 2020-07-04T11:11:19Z

From my point of view the issue in #54 is not that much about returning NaNs per se, but about returning NaNs inconsistently, only in a tiny fraction of cases, so small, that it would easily pass a test suite of 100 random values. It is worth to change this behaviour a) to be consistent with the majority of cases, b) to match our own documentation: http://hackage.haskell.org/package/random-1.2.0/docs/System-Random-Stateful.html#g:14

On the other hand, uniformRM (-∞,+∞) returns NaN consistently and is clearly documented ibidem. I do not see much reason to change this behaviour.

Shimuuar · 2020-07-04T13:42:45Z

I just found this behavior mildly surprising which is fine for floating point. What isn't when it comes to floating point.

BTW I came up with optimization which exploits quirks of floating point arithmetics:

  | isInfinite l || isInfinite h = return $! h + l

If only one of l,h is infinite they'll absorb finite counterpart and (-∞) + (+∞) = NaN. It bring implementation from 4 ifs to only 2 ifs.

Otherwise LGTM

Bodigrim · 2020-12-01T23:27:23Z

@curiousleo what do you think about @Shimuuar's suggestion? Also, could you please rebase?

curiousleo · 2020-12-02T07:16:31Z

@curiousleo what do you think about @Shimuuar's suggestion? Also, could you please rebase?

Thanks for the reminder. The suggestion looks neat! I'll come back to this PR as soon as free time permits.

curiousleo commented Jun 25, 2020

View reviewed changes

curiousleo marked this pull request as ready for review June 25, 2020 09:56

curiousleo commented Jun 25, 2020

View reviewed changes

curiousleo changed the title ~~Infinities as floating point endpoints~~ Infinities as floating point range endpoints Jun 25, 2020

curiousleo requested review from Bodigrim, idontgetoutmuch and lehins June 25, 2020 10:08

curiousleo marked this pull request as draft June 25, 2020 10:16

curiousleo marked this pull request as ready for review June 25, 2020 11:18

curiousleo commented Jun 25, 2020

View reviewed changes

src/System/Random.hs Outdated Show resolved Hide resolved

curiousleo force-pushed the fp-infinities branch from 67e12bf to 2e80584 Compare June 25, 2020 12:11

Bodigrim approved these changes Jun 25, 2020

View reviewed changes

idontgetoutmuch reviewed Jun 26, 2020

View reviewed changes

lehins approved these changes Jun 26, 2020

View reviewed changes

Shimuuar approved these changes Jul 4, 2020

View reviewed changes

curiousleo added 3 commits December 2, 2020 17:51

Infinities as floating point endpoints

d7c0462

Optimise

7c48e36

Use uniform{Double,Float}01M for random

a18a273

curiousleo added 2 commits December 2, 2020 17:51

Retain backwards compatibility

9baee56

Optimise handling of infinities

ef90a0f

curiousleo force-pushed the fp-infinities branch from 2cbfe1b to ef90a0f Compare December 2, 2020 18:24

curiousleo merged commit 96957e5 into haskell:master Dec 4, 2020

curiousleo deleted the fp-infinities branch December 4, 2020 07:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Infinities as floating point range endpoints #68

Infinities as floating point range endpoints #68

curiousleo commented Jun 25, 2020 •

edited

Loading

curiousleo Jun 25, 2020

curiousleo Jun 25, 2020 •

edited

Loading

curiousleo commented Jun 25, 2020 •

edited

Loading

curiousleo commented Jun 25, 2020 •

edited

Loading

curiousleo commented Jun 25, 2020

Shimuuar commented Jun 25, 2020

curiousleo commented Jun 25, 2020

idontgetoutmuch Jun 26, 2020

Shimuuar Jun 26, 2020

lehins Jun 26, 2020

Bodigrim commented Jul 1, 2020 •

edited

Loading

Shimuuar commented Jul 3, 2020

Bodigrim commented Jul 3, 2020 •

edited

Loading

Bodigrim commented Jul 4, 2020 •

edited

Loading

Shimuuar commented Jul 4, 2020

Bodigrim commented Dec 1, 2020

curiousleo commented Dec 2, 2020

	-- * If \(a\) is @-Infinity@ and \(b\) is @Infinity@, the result is @NaN@.
	--
	-- >>> let (a, b, x) = (-inf, inf, 0.5) in x * a + (1 - x) * b
	-- NaN

Infinities as floating point range endpoints #68

Infinities as floating point range endpoints #68

Conversation

curiousleo commented Jun 25, 2020 • edited Loading

curiousleo Jun 25, 2020

Choose a reason for hiding this comment

curiousleo Jun 25, 2020 • edited Loading

Choose a reason for hiding this comment

curiousleo commented Jun 25, 2020 • edited Loading

curiousleo commented Jun 25, 2020 • edited Loading

curiousleo commented Jun 25, 2020

Shimuuar commented Jun 25, 2020

curiousleo commented Jun 25, 2020

idontgetoutmuch Jun 26, 2020

Choose a reason for hiding this comment

Shimuuar Jun 26, 2020

Choose a reason for hiding this comment

lehins Jun 26, 2020

Choose a reason for hiding this comment

Bodigrim commented Jul 1, 2020 • edited Loading

Shimuuar commented Jul 3, 2020

Bodigrim commented Jul 3, 2020 • edited Loading

Bodigrim commented Jul 4, 2020 • edited Loading

Shimuuar commented Jul 4, 2020

Bodigrim commented Dec 1, 2020

curiousleo commented Dec 2, 2020

curiousleo commented Jun 25, 2020 •

edited

Loading

curiousleo Jun 25, 2020 •

edited

Loading

curiousleo commented Jun 25, 2020 •

edited

Loading

curiousleo commented Jun 25, 2020 •

edited

Loading

Bodigrim commented Jul 1, 2020 •

edited

Loading

Bodigrim commented Jul 3, 2020 •

edited

Loading

Bodigrim commented Jul 4, 2020 •

edited

Loading