Move `random(inRange:)` to `Collection` extension #97

andrewdolce · 2016-11-22T15:54:24Z

Addresses issue #89.

This was a bit more complex than originally expected. The function is no longer defined on CountableClosedRange<Int> specifically, but applies to any range with Bound conforming to the SignedInteger protocol. The arithmetic also had to be altered slightly to account for the generic bound/stride type.

Updates after refactor:

Revised replacement for `ip_random(inRange:)`

The ip_random function now lives on an extension of Collection. This means we can still use it to return a random value within a numerical range (since ranges are collections) as well as in many other situations, like say grabbing a random element from an array.

The new unit tests remain the same as the first version of this PR: I introduced a way to provide an alternative RNG function instead of arc4random_uniform. This was done entirely for testing purposes, and I've left it out of the public API. See the new unit test code and comments for a longer explanation. I'm really interested in feedback as to whether others agree with the testing approach.

Replacement of `ip_random()` on `UnsignedInteger`

There is an additional ip_random function that can be called on any class conforming to UnsignedInteger, to return a random value in range [0, max]. A private library [name redacted] uses this to generate random values for UInt8. However the implementation is flawed in that it relies on arc4random_uniform which expects UInt32. So if the function is called on a larger type, like UInt64 (or UInt on a 64-bit device), the result is an overflow crash.

The easiest way to fix this without seeking out a new implementation is to restrict this function for use on UInt32 and under, which I've done by introducing a protocol adopted specifically by UInt8, UInt16, and UInt32.

brightredchilli

Love the change to signed integer.

brightredchilli · 2016-11-22T16:30:18Z

SwiftWisdom/Core/StandardLibrary/Numbers/CountableClosedRange+Extensions.swift

+
+import Foundation
+
+public extension CountableClosedRange where Bound: SignedInteger {


question here, does this impose any limitations on the stride then? can there be countableclosedrange of stride 2? from 0...4?

PS this is really nice.

It hasn't been tested with different strides. I will address that before merging.

Well, if there can indeed be different strides, then this is kind of worrying...we then need to make sure that the random output falls on the stride boundary, right? am I thinking about this right? 0...4 with stride 2 only had 0,2,4 as valid values.

brightredchilli · 2016-11-22T16:35:34Z

SwiftWisdomTests/StandardLibrary/Numbers/CountableClosedRange+ExtensionsTests.swift

+        })
+
+        guard let rngUpperBound = capturedRNGUpperBound, rngUpperBound > 0 else {
+            XCTAssert(false, "Expected that RNG function upper bound \(capturedRNGUpperBound) is non-nil and positive.")


I would rather we translated some items in the guard into a more readable assertion.

ie

XCTAssert(rngUpperBound > 0, "Upper bound is less than zero, expected more than zero")

Also, per guard style guide, we should not use them as control flow, but only for early return. In addition, i dont think if lets are better in tests.

brightredchilli · 2016-11-22T16:39:57Z

SwiftWisdomTests/StandardLibrary/Numbers/CountableClosedRange+ExtensionsTests.swift

+
+        for mockRNGValue in 0..<rngUpperBound {
+            let sample = testRange.ip_random(withUniformRNGFunction: { upperBound in
+                XCTAssertEqual(upperBound, rngUpperBound, "Expected RNG function to be called with a consistent upper bound each time." )


im slightly confused as to why this needs to be in a loop. mockRNGValue is not used in any assertion

I am returning mockRNGValue as the output of the mock RNG function. It's a stand-in for a possible value that arc4random would output. In other words, it's an input for ip_random.

Right, what im saying is that calling it in a loop doesn't actually test anything different.

brightredchilli · 2016-11-22T16:40:37Z

SwiftWisdomTests/StandardLibrary/Numbers/CountableClosedRange+ExtensionsTests.swift

+
+class CountableClosedRangeExtensionTests: XCTestCase {
+
+    // This test is complex. Here is an explanation as to why:


totallly agree with the reasoning outlined in these comments, but I think we need to pare it down a little :D

brightredchilli · 2016-11-22T16:41:11Z

SwiftWisdom/Core/StandardLibrary/Numbers/CountableClosedRange+Extensions.swift

@@ -0,0 +1,25 @@
+//
+//  CountableClosedRange+Extensions.swift


I feel like Ranges deserve their own home under StandardLibrary. Thoughts?

andrewdolce · 2016-11-23T04:31:14Z

@brightredchilli So in exploring the variable stride issue, I've come to the realization that (I think) ip_random could live inside an extension on Collection instead. Then we'd get random element access for all collections, including CountableClosedRange. Working on that now.

brightredchilli · 2016-11-29T16:01:28Z

@andrewdolce - it sounds like there is more work to be done there, maybe we can open an issue for that and get these guys checked in?

ghost · 2016-12-09T20:18:08Z

	1 Message
📖	Executed 95 tests, with 0 failures (0 unexpected) in 0.314 (0.395) seconds

Generated by 🚫 danger

andrewdolce · 2017-02-23T19:56:35Z

@brightredchilli @paulrolfe I updated this finally. Could you guys give it another look? I'd like Paul to weigh in on the UnsignedInteger extension change, since it potentially impacts BMAP. Thanks!

brightredchilli

I would advocate for not testing, but otherwise this is a good to go for me.

brightredchilli · 2017-02-24T15:18:19Z

SwiftWisdomTests/StandardLibrary/Numbers/RandomizableUnsignedIntegerTests.swift

+
+class RandomizableUnsignedIntegerTests: XCTestCase {
+    func testRandom() {
+        let _: UInt8 = UInt8.ip_random()


why let _? isn't _ = UInt8.ip_random() enough?

Whoops, I'll fix.

brightredchilli · 2017-02-24T15:19:24Z

SwiftWisdom/Core/StandardLibrary/Collection/Collection+Random.swift

+
+public extension Collection {
+    public func ip_random() -> Self.Iterator.Element {
+        return ip_random(withUniformRNGFunction: arc4random_uniform)


do we ever change this? if not can we just combine this and the below function?

I can remove this. The only purpose of it was to let the tests specify a mock RNG function.

brightredchilli · 2017-02-24T21:02:12Z

SwiftWisdomTests/StandardLibrary/Collection/Collection+RandomTests.swift

+
+class CollectionRandomTests: XCTestCase {
+
+    // This test is complex. Here is an explanation as to why:


I do think these tests are quite comprehensive but for the reasons that you have mentioned, i kind of think that we should not include them...they seem unwieldy to update and seem to be dancing around the 'actual' random test.

Fwiw, I ran this code:

var array = repeatElement(0, count: Int(UInt8.max)).ip_toArray() for _ in 0...1000000 { let index = UInt8.ip_random() array[Int(index)] = array[Int(index)] + 1 } print(array)

and got

[3927, 3857, 3833, 3904, 3933, 3932, 3956, 3889, 4012, 3949, 3924, 3888, 3871, 3890, 3891, 3900, 4003, 3945, 3804, 3898, 3934, 4019, 3976, 4001, 3885, 3893, 3940, 4007, 3872, 4012, 3954, 3971, 3829, 4060, 3953, 3850, 4010, 3873, 3963, 3979, 4003, 4006, 3922, 3836, 3893, 3958, 3799, 3873, 3894, 3976, 3840, 3890, 3964, 3916, 3917, 3978, 3918, 3816, 3925, 3880, 3974, 3914, 4006, 3945, 3748, 3888, 4020, 3876, 3981, 3994, 3941, 3809, 3962, 3929, 3922, 3854, 3928, 3940, 3978, 3930, 3912, 3864, 3818, 3983, 3895, 3869, 3859, 3972, 3972, 3905, 3948, 3992, 4049, 3936, 3914, 3895, 3931, 3985, 3882, 3895, 3926, 4041, 3865, 4000, 3965, 3987, 3853, 3987, 4011, 3823, 3815, 3902, 3968, 3940, 3818, 3866, 4001, 3974, 3866, 3980, 3952, 4053, 3890, 4031, 3896, 3843, 3857, 3893, 3905, 3967, 4091, 4035, 3918, 3881, 3826, 3955, 3898, 3896, 3854, 3966, 3854, 4006, 4012, 3878, 3957, 3914, 3922, 3934, 3885, 4037, 3962, 3937, 3848, 3920, 3834, 3860, 3859, 3858, 3966, 3900, 3948, 3929, 3856, 3925, 3824, 3888, 3934, 3848, 3980, 3935, 3942, 3923, 3934, 3871, 3886, 3867, 3886, 3904, 3886, 3960, 3848, 3890, 3871, 4010, 3846, 3962, 3919, 3897, 3884, 3897, 3953, 3901, 3826, 3899, 4008, 3965, 4018, 3999, 3870, 3938, 3853, 3887, 3948, 3889, 3860, 3951, 3920, 3882, 3916, 3913, 3875, 4023, 3944, 3956, 3851, 3921, 3926, 4018, 3916, 3955, 3910, 3842, 3908, 3915, 3935, 3899, 4005, 3975, 3947, 3884, 3869, 3983, 3831, 3931, 3926, 3897, 3927, 3980, 3937, 3953, 3927, 3889, 3849, 3981, 3929, 3929, 3943, 3872, 3993, 3902, 3837, 4080, 3837, 3894, 3810]

Not by any means a scientific test, but I feel good about it.

I see your point about maintainability. I'll take them out and leave in the original tests.

brightredchilli approved these changes Nov 22, 2016

View reviewed changes

paulrolfe mentioned this pull request Nov 23, 2016

UnsignedInteger+Extension + tests #98

Closed

andrewdolce self-assigned this Feb 14, 2017

Andrew Dolce added 5 commits February 17, 2017 20:24

Move global random(inRange:) to CountableClosedRange extension

0ea53b8

Revise ip_random() unit tests

c9ad9a9

Get random element from RandomAccessCollection

c80ed27

Refactor UnsignedInteger ip_random()

cd43b8b

Bump the podspec to 0.7.3

dd107d0

andrewdolce force-pushed the adolce/ip_random branch from 5543028 to dd107d0 Compare February 23, 2017 19:10

andrewdolce changed the title ~~Move random(inRange:) to CountableClosedRange extension~~ Move random(inRange:) to Collection extension Feb 23, 2017

brightredchilli approved these changes Feb 24, 2017

View reviewed changes

cprime closed this Feb 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move `random(inRange:)` to `Collection` extension #97

Move `random(inRange:)` to `Collection` extension #97

andrewdolce commented Nov 22, 2016 •

edited by paulrolfe

Loading

brightredchilli left a comment

brightredchilli Nov 22, 2016

brightredchilli Nov 22, 2016

andrewdolce Nov 22, 2016

brightredchilli Nov 22, 2016

brightredchilli Nov 22, 2016

brightredchilli Nov 22, 2016

brightredchilli Nov 22, 2016

andrewdolce Nov 22, 2016 •

edited

Loading

brightredchilli Nov 22, 2016

brightredchilli Nov 22, 2016

brightredchilli Nov 22, 2016

andrewdolce Nov 22, 2016

andrewdolce commented Nov 23, 2016

brightredchilli commented Nov 29, 2016

ghost commented Dec 9, 2016

andrewdolce commented Feb 23, 2017

brightredchilli left a comment

brightredchilli Feb 24, 2017

andrewdolce Feb 28, 2017

brightredchilli Feb 24, 2017

andrewdolce Feb 28, 2017

brightredchilli Feb 24, 2017

andrewdolce Feb 28, 2017


		import Foundation

		public extension CountableClosedRange where Bound: SignedInteger {


		class CountableClosedRangeExtensionTests: XCTestCase {

		// This test is complex. Here is an explanation as to why:


		class CollectionRandomTests: XCTestCase {

		// This test is complex. Here is an explanation as to why:

Move random(inRange:) to Collection extension #97

Move random(inRange:) to Collection extension #97

Conversation

andrewdolce commented Nov 22, 2016 • edited by paulrolfe Loading

Updates after refactor:

Revised replacement for ip_random(inRange:)

Replacement of ip_random() on UnsignedInteger

brightredchilli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewdolce Nov 22, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewdolce commented Nov 23, 2016

brightredchilli commented Nov 29, 2016

ghost commented Dec 9, 2016

andrewdolce commented Feb 23, 2017

brightredchilli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Move `random(inRange:)` to `Collection` extension #97

Move `random(inRange:)` to `Collection` extension #97

andrewdolce commented Nov 22, 2016 •

edited by paulrolfe

Loading

Revised replacement for `ip_random(inRange:)`

Replacement of `ip_random()` on `UnsignedInteger`

andrewdolce Nov 22, 2016 •

edited

Loading