Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DO NOT MERGE] [stdlib] Random unification #12772

Closed
wants to merge 40 commits into from

Conversation

Azoy
Copy link
Contributor

@Azoy Azoy commented Nov 5, 2017

This is the implementation for swiftlang/swift-evolution#760

1. Fixed an issue where integers would would result in an infinite loop if they were unsigned, or signed integers always returning negative numbers.
2. Fixed an issue with Bool initialization
Fixes more typos

Also states that the range for floating points is from 0 to 1 inclusive
@gparker42
Copy link
Contributor

Randomness distribution tests would be appropriate for the long tests.

Other basic tests are desirable: does the API work at all; if you ask for two large random numbers are they different; if you ask for a bunch of random numbers within a very small range do they all fall within that range.

void swift::_swift_stdlib_random(void *buf,
__swift_ssize_t nbytes,
__swift_uint32_t debug_flags) {
if (SecRandomCopyBytes(kSecRandomDefault, nbytes, buf) != 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SecRandomCopyBytes() should be available on all of the Apple platforms supported by Swift. I don't think you need the arc4random() or /dev/urandom implementations there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

arc4random was suggested by Ben Cohen on the evolution mailing list here: https://lists.swift.org/pipermail/swift-evolution/Week-of-Mon-20171002/040195.html . I can substitute SecRandomCopyBytes() instead of reading /dev/urandom.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding is that SecRandomCopyBytes() is cryptographically superior to arc4random() on old Apple OS versions. I see no reason not to use it on all versions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

arc4random(3) was replaced in macOS 10.12 and iOS 10 to provide a more secure generator. I do indeed have the version checks to make sure to use arc4random(3) on those OS versions on and SecRandomCopyBytes() for older versions.

Remove unnecessary import

Make sure not to return upperBound on Range

Use SecRandomCopyBytes on older macOS
@Azoy Azoy force-pushed the random-unification branch 2 times, most recently from 8b900b5 to 08cd01b Compare November 23, 2017 04:03
@Azoy
Copy link
Contributor Author

Azoy commented Nov 23, 2017

Added tests!

Added pick(_:) to collection
Added random(in:using:) to Randomizable
Added tests

Fix typo in Randomizable documentation

Rename pick to sampling

Move sampling below random
@natecook1000 natecook1000 added the swift evolution pending discussion Flag → feature: A feature that has a Swift evolution proposal currently in review label Dec 7, 2017
Fix FloatingPoint initialization

Precondition getting a random number from range
Copy link
Contributor

@benrimmington benrimmington left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's a quick review of the documentation.

@@ -87,6 +87,30 @@ public struct Bool {
public init(_ value: Bool) {
self = value
}

/// Returns a random Boolean
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be "Boolean value." to match the rest of the file?

///
/// - Parameter generator: The random number generator to use when getting a
/// random Boolean.
/// - Returns: A random Boolean.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto x2: "Boolean value."

return (generator.next() >> 17) & 1 == 0
}

/// Returns a random Boolean
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto x2: "Boolean value."

/// Returns a random Boolean
///
/// - Parameter generator: The random number generator to use when getting a
/// random Boolean.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function doesn't have a generator parameter.

/// `numbers.shuffled(using: &Random.default)`.
///
/// - Parameter generator: The random number generator to use when shuffling
/// the sequence.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function doesn't have a generator parameter.

/// - Parameter generator: The random number generator to use when shuffling
/// the collection.
///
/// - Complexity: O(*n*)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this determined by the complexity of count (which is accessed twice) and index(_:offsetBy:), which are both O(n) or O(1)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm. If the collection isn't random-access, this ends up being O(n^2), since index(_:offsetBy:) gets called n times. For random-access collections, it's O(n), since there are count - 1 swaps.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Accessing the count property can be O(n), so could you store it locally with name shadowing?

let count = self.count
guard count > 1 else { return }
// etc.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's an optimization that should be added at some point.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@natecook1000 for in-place shuffling, should I change the complexity to O(n^2)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or should I do something like this:

/// - Complexity: O(*n*) if the collection conforms to
///   `RandomAccessCollection`; otherwise, O(*n^2*), where *n* is the length
///   of the collection.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest, I was thinking we should probably give it the same constraints as sort, which is to add RandomAccessCollection as a requirement for the in-place shuffling methods.

@airspeedswift What do you think? Does parity with sort make sense?

/// the sequence.
/// - Returns: An array of this sequence's elements in a shuffled order.
///
/// - Complexity: O(*n*)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto: see MutableCollection.shuffle(using:) complexity?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The nonmutating versions will always be O(n), since it copies the sequence into an array first, which is random-access.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does it copy the sequence into a ContiguousArray, shuffle that, and then copy the result into an Array?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The copy into ContiguousArray forces an eager bridge if the sequence is an NSArray. The copy into Array at the end reuses the same buffer, so it kind of just acts like a cast.

/// the sequence.
/// - Returns: A shuffled array of this sequence's elements.
///
/// - Complexity: O(*n*)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto: see MutableCollection.shuffle(using:) complexity?

/// to `names.shuffle()` above is equivalent to calling
/// `names.shuffle(using: &Random.default)`.
///
/// - Complexity: O(*n*)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto: see MutableCollection.shuffle(using:) complexity?

}

public // @testable
func _stdlib_random(_ bytes: UnsafeMutableRawBufferPointer) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to decide how to test this internal API.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@benrimmington A naive way to test this would be to have two buffers equal to each other, and perform this API on one of them to see if it still equals the starting state. Then do the same to the other buffer, and check if it doesn't equal the other buffer. I don't know if this is a great way to test it, but it seems reasonable to me at least.

/// // Prints "14.2286325689993"
/// // Prints "13.1485686260762"
///
/// The `random(using:)` static method chooses a random value from a
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

random(using:) => random(in:using:)

@benrimmington
Copy link
Contributor

Should this pull request be @inlinable audited, or will that happen later for Swift 5?

@Azoy Azoy force-pushed the random-unification branch 2 times, most recently from 7763177 to a33ffeb Compare April 24, 2018 04:18
}

// buffer1 == buffer2
expectTrue(buffer1.elementsEqual(buffer2))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should initialize these buffers before comparing their contents.

Copy link
Contributor

@benrimmington benrimmington Apr 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

var zeros = [UInt8](repeating: 0, count: 10)

for count in [100, 1000] {
    // Initialize two large buffers.
    var bytes1 = [UInt8](repeating: 0, count: count)
    var bytes2 = [UInt8](repeating: 0, count: count)
    expectEqual(bytes1, bytes2)
    expectNotNil(memmem(&bytes1, bytes1.count, &zeros, zeros.count))
    expectNotNil(memmem(&bytes2, bytes2.count, &zeros, zeros.count))

    // Fill with random bytes.
    bytes1.withUnsafeMutableBytes({ _stdlib_random($0) })
    bytes2.withUnsafeMutableBytes({ _stdlib_random($0) })
    expectNotEqual(bytes1, bytes2)
    expectNil(memmem(&bytes1, bytes1.count, &zeros, zeros.count))
    expectNil(memmem(&bytes2, bytes2.count, &zeros, zeros.count))
}

I've used memmem in this example, but is there a stdlib API to find a subsequence?

Edit: Foundation.Data has a range(of:options:in:) method, but its withUnsafeMutableBytes(_:) method is wrong.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's a unit test which also shows how _stdlib_random might work in the Foundation overlay.

import Foundation

extension Data {
  static func _random(count: Int) -> Data {
    var result = Data(count: count)
    result.withUnsafeMutableBytes {
      (start: UnsafeMutablePointer<UInt8>) -> Void in
      let start = UnsafeMutableRawPointer(start)
      let bytes = UnsafeMutableRawBufferPointer(start: start, count: count)
      _stdlib_random(bytes)
    }
    return result
  }
}

let zeros = Data(count: 10)
for count in [100, 1000] {
  let data1 = Data._random(count: count)
  let data2 = Data._random(count: count)
  expectNil(data1.range(of: zeros))
  expectNil(data2.range(of: zeros))
  expectNotEqual(data1, data2)
}

Should it actually be _swift_stdlib_random with the SWIFT_RUNTIME_STDLIB_SPI attribute?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can keep it _stdlib_random with SWIFT_RUNTIME_STDLIB_INTERNAL atm because I plan to follow up with another proposal to fill buffers. The solution atm isn’t optimal (calling Random.default.next() many times), but it lets us keep stdlib visibility. In your memmem example, couldn’t you call elementsEqual(_:) because you’re checking if all elements are equal in sequential order?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Sequence.elementsEqual will iterate both sequences, even if they have a different number of elements.

  • Array.== returns early if the count is different, or if the buffer identity is the same.

  • Data.== also returns early, and uses the memcmp API (which might be faster).

In this example, the arrays have the same number of elements in different buffers, so elementsEqual and == will do the same thing.

The memmem API might not be available on all platforms.

let random = generator.next(upperBound: UInt(count))
let index = self.index(
startIndex,
offsetBy: numericCast(random)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of using next(upperBound:) and numericCast(_:), would the following work?

let randomDistance = Int.random(in: 0 ..< count, using: &generator)
let randomIndex = index(startIndex, offsetBy: randomDistance)
return self[randomIndex]

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, this would be indirectly calling Range.randomElement(using:) in the default implementation of Collection.randomElement(using:), which is a protocol requirement.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@benrimmington The changes you proposed worked, I'm not sure if you want to proceed with it however.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indirectly calling Range.randomElement(using:) inside Collection.randomElement(using:) won't be recursive. But will it be slower than next(upperBound:) and numericCast(_:)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My initial reaction is yes it will be slower, but it should be minuscule however.

@Azoy Azoy force-pushed the random-unification branch 2 times, most recently from 308dbfc to cf944cb Compare April 24, 2018 23:23
var currentIndex = startIndex
while amount > 1 {
let random = generator.next(upperBound: UInt(amount))
amount -= 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatives:

  • var amount = count; while amount > 1 { amount -= 1 }
  • for amount in stride(from: count, through: 2, by: -1) {}
  • for amount in (2 ... count).reversed() {}

Ditto: next(upperBound:) and numericCast(_:)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any benefit to using an alternative here rather than the first one that you pointed out (which is what is currently implemented)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only benefit is less lines of code, and possibly the intention is more obvious.

@@ -308,3 +318,67 @@ swift::_stdlib_cxx11_mt19937_uniform(__swift_uint32_t upper_bound) {
std::uniform_int_distribution<__swift_uint32_t> RandomUniform(0, upper_bound);
return RandomUniform(getGlobalMT19937());
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could add a reminder here to update the default RandomNumberGenerator documentation (if _stdlib_random is changed).

@Azoy
Copy link
Contributor Author

Azoy commented Apr 26, 2018

@benrimmington @natecook1000 are there anymore implementation details that we still need to work out (Besides hearing a response from @airspeedswift regarding constraining the shuffle implementation to RandomAccessCollection to have parity with sort)?

@benrimmington
Copy link
Contributor

I've already asked about adding the generic next() and next(upperBound:) as protocol requirements.

But the problem is those APIs aren't tested with types larger than UInt64, and even using DoubleWidth won't test the condition where T.bitWidth.quotientAndRemainder(dividingBy: UInt64.bitWidth) returns a non-zero remainder.

I also wonder if next(upperBound:) could be private API, because it seems unlikely that a generator will be able to implement it differently. For example, the arc4random_uniform API can't be used, because next(upperBound:) has a generic return type.

I haven't reviewed BinaryFloatingPoint.random(in:using:) or Range.randomElement(using:) because I don't understand them.

@inlinable
public mutating func next<T: FixedWidthInteger & UnsignedInteger>(
upperBound: T
) -> T {
Copy link
Contributor

@benrimmington benrimmington Apr 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need one of the following?

  • guard upperBound != 0 else { return 0 }
  • _precondition(upperBound != 0, "Can't get random value with lowerBound == upperBound")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I think the first one would make the most sense rather than trapping. Thanks!

@Azoy Azoy force-pushed the random-unification branch 2 times, most recently from 45efe77 to f7b527b Compare April 27, 2018 03:30
@Azoy
Copy link
Contributor Author

Azoy commented Apr 27, 2018

We can't move next() or next(upperBound:) to requirements yet. I've asked Ben and he said it would be best to follow up with another proposal (including something like fillBuffer(_:) which I'm writing up now).

As for testing next(), to me it seems unreasonable to even expect someone to create something like UInt83, maybe I'm wrong 🤔 . I think once DoubleWidth gets released we should add new tests make sure those get filled up.

@benrimmington
Copy link
Contributor

@inlinable
public func randomElement<T: RandomNumberGenerator>(
using generator: inout T
) -> Element? {
Copy link
Contributor

@benrimmington benrimmington Apr 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Edit: renamed the internal API.

I think the following might be possible, to avoid the GYB code.

extension ${Range} where
  Bound: FixedWidthInteger,
  Bound.Stride: SignedInteger,
  Bound.Magnitude: UnsignedInteger
{

  internal var _count: Bound.Magnitude? {
    if contains(.min) && contains(.max) {
      return nil
    }
    let count: Bound.Magnitude
    switch (lowerBound.signum(), upperBound.signum()) {
    case (-1, -1):
      count = lowerBound.magnitude - upperBound.magnitude
    case (1, 1):
      count = upperBound.magnitude - lowerBound.magnitude
    default:
      count = lowerBound.magnitude + upperBound.magnitude
    }
    return contains(upperBound) ? count + 1 : count
  }

  public func randomElement<T: RandomNumberGenerator>(
    using generator: inout T
  ) -> Element? {
    guard !isEmpty else {
      return nil
    }
    let randomDistance: Bound.Magnitude
    if let maxDistance: Bound.Magnitude = _count {
      randomDistance = generator.next(upperBound: maxDistance)
    } else {
      randomDistance = generator.next()
    }
    return lowerBound &+ Bound(truncatingIfNeeded: randomDistance)
  }
}

The overflow addition is a bit obscure, but it seems to work.

Is there a Bound.bitWidth == Bound.Magnitude.bitWidth guarantee?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to treat avoiding GYB in a GYB'd file as a goal, particularly if it ends up adding runtime branches. If it makes sense to extract some of this to a common routine, that's great, but GYB should generate the code you would write if you were writing out each version specifically. (Here and below.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@natecook1000, I find it difficult to read the GYB version, when it contains lots of %ifs inside the function body. I don't think I've added any extra branches.

range.lowerBound != range.upperBound,
"Can't get random value with lowerBound == upperBound"
)
% end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of comparing bounds inside a GYB block, you might be able to use:

_precondition(!range.isEmpty, "Can't get random value with empty range")

With closed ranges, the isEmpty property is always false.

range.lowerBound != range.upperBound,
"Can't get random value with lowerBound == upperBound"
)
% end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto:

_precondition(!range.isEmpty, "Can't get random value with empty range")


// Basic random numbers

RandomTests.test("basic random numbers") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this also test the full ranges (min to max) of Int8 and UInt8?

The number of iterations would need to increase (to 100_000 or 1_000_000 perhaps).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I'm following when you say "test the full ranges".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I attached the previous comment to the wrong test. I meant the "random integers in ranges" test, using the integerRangeTest helper function. It's currently testing for the min/max ± 10, but it might be good to also test for the entire 256 elements of the 8-bit closed ranges (and maybe half-open ranges).

  • (Int8.min ... .max) and (UInt8.min ... .max)
  • (Int8.min ..< .max) and (UInt8.min ..< .max)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've done a few tests with 10_000 iterations and it hasn't failed once, so I'm going to set it to that initially. Let me know if we still need to increase it just in case.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might be able to simplify integerRangeTest by adding a couple more helper functions (one taking a half-open range, another taking a closed range). The number of iterations could then be for _ in 0 ..< (100 * range.count) for example. The body of the existing integerRangeTest would be:

integerRangeTest(in: T.min ..< (T.min + 10))
integerRangeTest(in: T.min ... (T.min + 10))
integerRangeTest(in: (T.max - 10) ..< T.max)
integerRangeTest(in: (T.max - 10) ... T.max)
if T.bitWidth == 8 {
  integerRangeTest(in: T.min ..< T.max)
  integerRangeTest(in: T.min ... T.max)
}

By the way, all the recent changes have been squashed into the "Consolidate _stdlib_random functions" commit.

* Use the `__has_include` and `GRND_RANDOM` macros

* Use `getentropy` instead of `getrandom`

* Use `std::min` from the <algorithm> header

* Move `#if` out of the `_stdlib_random` function

* Use `getrandom` with "/dev/urandom" fallback

* Use `#pragma comment` to import "Bcrypt.lib"

* <https://docs.microsoft.com/en-us/cpp/preprocessor/comment-c-cpp>
* <https://clang.llvm.org/docs/UsersManual.html#microsoft-extensions>

* Use "/dev/urandom" instead of `SecRandomCopyBytes`

* Use `swift::StaticMutex` for shared "/dev/urandom"

* Add `getrandom_available`; use `O_CLOEXEC` flag

Add platform impl docs

Update copyrights

Fix docs

Add _stdlib_random test

Update _stdlib_random test

Add missing &

Notice about _stdlib_random

Fix docs

Guard on upperBound = 0

Test full range of 8 bit integers

Remove some gyb
% exampleRange = '10.0..<20.0' if Range == 'Range' else '10.0...20.0'
extension BinaryFloatingPoint
where Self.RawSignificand : FixedWidthInteger,
Self.RawSignificand.Stride : SignedInteger & FixedWidthInteger,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does the RawSignificand.Stride need to be FixedWidthInteger?

public static func random<T: RandomNumberGenerator>(
in range: ${Range}<Self>,
using generator: inout T
) -> Self {
Copy link
Contributor

@benrimmington benrimmington May 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's another attempt at removing GYB code.

extension BinaryFloatingPoint where ... {

  public static func random<T: RandomNumberGenerator>(
    in range: ${Range}<Self>,
    using generator: inout T
  ) -> Self {
    _precondition(
      !range.isEmpty,
      "Can't get random value with an empty range"
    )
    let unitRandom = _unitRandom(using: &generator)
    let lowerBound = range.lowerBound
    let upperBound = range.contains(range.upperBound)
      ? range.upperBound
      : range.upperBound.nextDown
    return (upperBound - lowerBound) * unitRandom + lowerBound
  }
}

The internal API would be outside the % for Range in ['Range', 'ClosedRange']: block.

extension BinaryFloatingPoint where ... {

  // Returns a random value in the `0.0 ... 1.0` unit interval,
  // using raw values which can be represented exactly.
  internal static func _unitRandom<T: RandomNumberGenerator>(
    using generator: inout T
  ) -> Self {
    let max = RawSignificand(1.0 / ulpOfOne)
    let raw = RawSignificand.random(in: 0 ... max, using: &generator)
    return Self(exactly: raw)! / Self(exactly: max)!
  }
}

The significandBitCount can be Int.max for extensible floating-point types. I'm not sure if we need to worry about that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think 1.0 / ulpOfOne gives the maximum precision needed.

-    let max = (significandBitCount == RawSignificand.bitWidth - 1)
-      ? RawSignificand.max
-      : (1 as RawSignificand) << (significandBitCount + 1)
+    let max = RawSignificand(1.0 / ulpOfOne)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let upperBound = range.contains(range.upperBound)
      ? range.upperBound
      : range.upperBound.nextDown
    return (upperBound - lowerBound) * unitRandom + lowerBound

I'm not sure that logic works. Given 0.0 ..< 2.0, upperBound = 1 so (1 - 0) * unit + 0 will only produce values within 0.0 ... 1.0 whereas we need values from 0.0 ..< 2.0.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Azoy, range.contains(range.upperBound) is testing for a closed range; and range.upperBound.nextDown converts a half-open range into a closed range.

Given 0.0 ..< 2.0, the local upperBound and the delta will be 1.9999999999999998; I've tested this and it appears to work as intended.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh my apologies, I assumed nextDown would return the next integer down.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The non-branching version would be:

extension BinaryFloatingPoint where ... {

  public static func random<T: RandomNumberGenerator>(
    in range: ${Range}<Self>,
    using generator: inout T
  ) -> Self {
    _precondition(
      !range.isEmpty,
      "Can't get random value with an empty range"
    )
    let unitRandom = _unitRandom(using: &generator)
    let lowerBound = range.lowerBound
%   if 'Closed' in Range:
    let upperBound = range.upperBound
%   else:
    let upperBound = range.upperBound.nextDown
%   end
    return (upperBound - lowerBound) * unitRandom + lowerBound
  }
}

@Azoy Azoy closed this May 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
swift evolution approved Flag → feature: A feature that was approved through the Swift evolution process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet