[SR-8178] Fix BinaryFloatingPoint.random(in:) open range returning upperBound #17794
Conversation
|
cc: @stephentyrone |
Azoy
changed the title
| if randFloat == range.upperBound { | ||
| return randFloat.nextDown | ||
| } | ||
| % end |
There was a problem hiding this comment.
This introduces a (very slight) bias, as it effectively makes the penultimate value 1.5 times more likely to occur than every other value in the binade. As a hacky quick fix to preserve the desired invariant, it's semi-acceptable, but let's see if we can get a better long-term fix done instead.
There was a problem hiding this comment.
I'll either make a patch myself or send you some notes on how to do this in a more principled way later today.
There was a problem hiding this comment.
Sounds good to me. Would love to get some notes even if you patch it yourself if that's fine.
There was a problem hiding this comment.
Absolutely. Thanks for adding the new tests, by the way; do you want to amend this PR to just the tests, and we'll land those as an expected-fail in the meantime?
There was a problem hiding this comment.
Actually, why don't you amend this patch as follows: instead of mapping upperBound to the next smaller value, simply re-draw if upperBound is generated. This is slightly inefficient, but completely eliminates the bias.
There was a problem hiding this comment.
If we re-draw with the same max rng, wouldn't we be forever re-drawing since upperBound would always be occurring?
There was a problem hiding this comment.
Yeah, that is a concern, though it won't happen with non-pathological RNGs =)
Maybe for the test use an RNG that instead generates [.min, .max, basic LCG sequence ...].
|
@swift-ci please smoke test. |
* [SR-8178] Fix BinaryFloatingPoint.random(in:) open range returning upperBound (#17794) * Require .upperBound - .lowerBound be finite for FloatingPoint random (#17833) This is a slightly conservative precondition; when we re-work the FloatingPoint random computation in a more principled fashion, we can relax this to only requiring that .upperBound and .lowerBound are both finite. However, the current computation will break down unless this conservative condition is used, and this is future proof--we will only relax it going forward.
The problem seemed to come from the final addition
+ range.lowerBound.