Skip to content

[SR-3887] Decimal(n) fails to initialise properly for many integers within range. #4110

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

Closed
swift-ci opened this issue Feb 7, 2017 · 8 comments

Comments

@swift-ci
Copy link
Contributor

swift-ci commented Feb 7, 2017

Previous ID SR-3887
Radar None
Original Reporter j-h-a (JIRA User)
Type Bug
Status Resolved
Resolution Done

Attachment: Download

Environment

Apple Swift version 3.0.2 (swiftlang-800.0.63 clang-800.0.42.1).

Additional Detail from JIRA
Votes 0
Component/s Foundation
Labels Bug
Assignee None
Priority Medium

md5: 3592da22040f9427c02f2ef22b03d3ed

Issue Description:

For many numbers easily representable by Decimal, the initialiser fails.

Example 1:

let a1 = Decimal(18446742) // 18446742.000000002048 (wrong)
let a2 = Decimal(18446741) + Decimal(1) // 18446742 (correct)

Example 2:

let b1 = Decimal(1000000007) // 1000000007.0000001024 (wrong)
let b2 = Decimal(1000000006) + Decimal(1) // 1000000007 (correct)

The following code calculates how many integers get initialised incorrectly in a few different ranges, all within the range of Int32:

let startValues = [
            0,
   59_386_023,
  264_109_865,
  364_109_865,
1_642_385_017,
]
let len = 10_000_000

for start in startValues {
    var fails = 0
    for i in start ..< (start + len) {
        let a = Decimal(i)
        if a.description.range(of: ".") != nil {
            fails += 1
        }
    }
    let percentFailed = 100.0 * Double(fails) / Double(len)
    print("Starting at \(start): Found \(fails) fails in \(len) (\(percentFailed)%)")
}

And the output is:

Starting at 0: Found 1106804 fails in 10000000 (11.06804%)
Starting at 59386023: Found 0 fails in 10000000 (0.0%)
Starting at 264109865: Found 0 fails in 10000000 (0.0%)
Starting at 364109865: Found 1293746 fails in 10000000 (12.93746%)
Starting at 1642385017: Found 6875000 fails in 10000000 (68.75%)
@swift-ci
Copy link
Contributor Author

swift-ci commented Feb 7, 2017

Comment by Jay Abbott (JIRA)

Possibly related to SR-3125 and SR-3130 - although they seem to be about larger numbers, above 2^29, whereas this issue starts as low as 1,475,741:

Decimal(1475741) // 1475740.9999999997952

Possibly of interest to Eridius (JIRA User) and @xwu.

@xwu
Copy link
Contributor

xwu commented Feb 7, 2017

Irksome. Possibly because it's being initialized via init(_: Double). Will check when I have some spare time.

@lilyball
Copy link
Mannequin

lilyball mannequin commented Feb 7, 2017

I only really tested the numbers around 1 << 29. I can certainly believe that there are lower numbers that have the same issue.

@xwu SR-3125 is about it being init(_: Double), but doubles can represent integers precisely up to 1 << 53, so that doesn't explain why the code is producing bad values for anything lower than that, which is what SR-3130 and this ticket are about.

@swift-ci
Copy link
Contributor Author

swift-ci commented Feb 8, 2017

Comment by Jay Abbott (JIRA)

I think this probably is a duplicate of SR-3130, or at least has the same internal cause. That report is about the Double initialiser but maybe it is just that less information was known, and it's actually the same thing (I have only tested initialising with Int values). I noticed that there is a consistent percentage of failures across some ranges. This heatmap image shows this for the integers from 0 to 2^26-1:

Each pixel represents 1024 contiguous samples, the brightness of the pixel represents the percentage of failures in those 1024 samples. The bands show that a consistent percentage of failures occur in ranges that increase in size and distance apart as you go further from zero. I also tried to map common bits in the integers that caused failed initialisations, but found no pattern.

Code to generate the heatmap:

import Foundation
import CoreGraphics

let samplesPerPixel = 1024
let width = 256
let height = 256
var heatmap = UnsafeMutablePointer<UInt8>.allocate(capacity: width * height)

var counter = 0
for y in 0 ..< height {
    for x in 0 ..< width {

        var sum = 0
        for i in 0 ..< samplesPerPixel {
            let a = Decimal.init(counter)
            if a.description.range(of: ".") != nil {
                sum += 1
            }
            counter += 1
        }
        let heat = UInt8(255.0 * Double(sum) / Double(samplesPerPixel - 1))
        heatmap[y * height + x] = heat
    }
}

let provider = CGDataProvider(dataInfo: nil, data: heatmap, size: width * height, releaseData: { (a, b, c) in })!
let img = CGImage(width: width, height: height,
                  bitsPerComponent: 8, bitsPerPixel: 8,
                  bytesPerRow: width,
                  space: CGColorSpaceCreateDeviceGray(),
                  bitmapInfo: .byteOrderMask,
                  provider: provider,
                  decode: nil,
                  shouldInterpolate: false,
                  intent: .defaultIntent)!
let url = NSURL(fileURLWithPath: "Decimal-failure-heatmap.png")
let dest = CGImageDestinationCreateWithURL(url, kUTTypePNG, 1, nil)!
CGImageDestinationAddImage(dest, img, nil)
CGImageDestinationFinalize(dest)

@lilyball
Copy link
Mannequin

lilyball mannequin commented Feb 8, 2017

j-h-a (JIRA User) As documented in SR-3125, the integral initializers all internally call init(_: Double), which is a problem because a double cannot represent the full range of Int64 and UInt64 (specifically, that issue calls out just those two initializers, because in theory there's nothing wrong with using init(_: Double) for e.g. init(_: Int32), as long as init(_: Double) is implemented correctly for all integral values that can be precisely represented in a double).

@swift-ci
Copy link
Contributor Author

swift-ci commented Feb 8, 2017

Comment by Jay Abbott (JIRA)

I see - yeah almost certainly a duplicate then.

@spevans
Copy link
Contributor

spevans commented Jun 3, 2020

This has been fixed in both macOS and Linux versions as the initialiser now avoids the conversion to Double

For sclf it was fixed in: #1550

and for macOS the fix was in swiftlang/swift#25745

Running the above test program on both macOS and Linux outputs:

Starting at 0: Found 0 fails in 10000000 (0.0%)
Starting at 59386023: Found 0 fails in 10000000 (0.0%)
Starting at 264109865: Found 0 fails in 10000000 (0.0%)
Starting at 364109865: Found 0 fails in 10000000 (0.0%)
Starting at 1642385017: Found 0 fails in 10000000 (0.0%)

@lilyball
Copy link
Mannequin

lilyball mannequin commented Jun 10, 2020

The macOS fix was actually swiftlang/swift#18486

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
@shahmishal shahmishal transferred this issue from swiftlang/swift May 5, 2022
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants