[minor]: Binomial guarantees#15
Conversation
There was a problem hiding this comment.
Code Review
This pull request adds validation to the Binomial subscript to ensure p is finite, adds documentation for the sample method, and introduces a new test suite BinomialGuarantees to verify the sampling range. The review feedback suggests adding a guard to prevent negative values of n in the subscript to avoid potential runtime crashes, and expanding the test suite to cover boundary edge cases like n = 0, p = 0, and p = 1.
| @inlinable public static subscript(n: Int64, p: Double) -> Self { | ||
| guard p.isFinite else { | ||
| fatalError("Binomial.subscript cannot be called with `p` = \(p)") | ||
| } | ||
| return .init(n: n, p: p) | ||
| } |
There was a problem hiding this comment.
If n is negative, calling pdf(_:) will attempt to construct an invalid range 0 ... self.n, which triggers a runtime fatal error in Swift (Can't form Range with upperBound < lowerBound). Guarding that n >= 0 in the subscript prevents this crash and ensures the distribution is always initialized in a valid state.
| @inlinable public static subscript(n: Int64, p: Double) -> Self { | |
| guard p.isFinite else { | |
| fatalError("Binomial.subscript cannot be called with `p` = \(p)") | |
| } | |
| return .init(n: n, p: p) | |
| } | |
| @inlinable public static subscript(n: Int64, p: Double) -> Self { | |
| guard n >= 0 else { | |
| fatalError("Binomial.subscript cannot be called with negative `n` = \(n)") | |
| } | |
| guard p.isFinite else { | |
| fatalError("Binomial.subscript cannot be called with `p` = \(p)") | |
| } | |
| return .init(n: n, p: p) | |
| } |
| @Test mutating func OutputRange() { | ||
| let n: Int64 = Int64.max - 1 | ||
| let distribution = Binomial[n, 1.nextDown] | ||
| for _: Int in 0 ..< 1000 { | ||
| let result: Int64 = distribution.sample(using: &self.random.generator) | ||
| #expect(result >= 0) | ||
| #expect(result <= n) | ||
| } | ||
| } |
There was a problem hiding this comment.
To thoroughly test the binomial guarantees and boundary conditions, consider adding test cases for edge cases such as n = 0, p = 0, and p = 1.
@Test mutating func OutputRange() {
let n: Int64 = Int64.max - 1
let distribution = Binomial[n, 1.nextDown]
for _: Int in 0 ..< 1000 {
let result: Int64 = distribution.sample(using: &self.random.generator)
#expect(result >= 0)
#expect(result <= n)
}
// Test boundary edge cases
#expect(Binomial[0, 0.5].sample(using: &self.random.generator) == 0)
#expect(Binomial[10, 0.0].sample(using: &self.random.generator) == 0)
#expect(Binomial[10, 1.0].sample(using: &self.random.generator) == 10)
}
document and test Binomial guarantees