BDouble.DecimalExpansion produces incorrect results for some BDouble values unless precision is set higher
Ben-Henshall opened this issue · 6 comments
It appears that for some values of BDouble, decimal expansion produces incorrect values below certain precision points.
For example, I added the following test case to the project:
bigD?.decimalExpansion(precisionAfterComma: 2)
is expected to equal "0.71"
, but instead resolves to "0.00".
This is also the case for any precision value up to 9, where the decimalExpansions resolves to what it is expected to be:
I don't know enough about the maths side of this to fix it, but I had a quick look at the decimalExpansion
function and found this block of code:
if res.count < digits + 2
{
let pad = String(repeating: "0", count: max(digits, 1))
res = res.padding(toLength: digits+2-origRes.count, withPad: pad, startingAt: res.count)
}
Removing that res.count < digits + 2
check fixes this issue, but I'm guessing it's required for some other reason?
All the tests in the project pass if this block is removed.
Hi! I fixed the issue, it should return the proper decimal expansion now. I also added a test that generates random "decimal expansions", converts them to DBouble, and den extracts the original value with the decimalExpansion function. I would appreciate some more tests, since I'm not 100% sure that my test covers every case
I don't think it covers enough cases since the number of decimal places used in the decimalExpansion is the same as the number of decimal places provided in the BDouble used. In other words, you're never reducing precision to cut off some (relevant) decimal places.
I think using tests where the length of the decimalExpansion string is larger/smaller than the origin BDouble string would be valid, e.g. check if (With random numbers substituted of course):
BDouble("1234.1234").decimalExpansion(precisionAfterComma: 2) == "1234.12"
or
BDouble("1234.1234").decimalExpansion(precisionAfterComma: 6) == "1234.123400"
If you think this is a valid testing method, I can go ahead and put in a PR for it.
I also noticed that there seems to be a precision test failing in master:
bigD = BDouble("0.0000000003") // nine zeroes
bigD?.precision = 0
XCTAssert(bigD?.decimalDescription == "0.0", (bigD?.decimalDescription)!)
I believe the decimal description should resolve to "0" if 0 precision is specified?
Thanks for all your work,
Ben.
I apologize for these errors as they are most likely mine. I am glad to see that other people are using this library. More tests are always appreciated.
Personally I'd rather 0.716x with a precision of 2 be 0.72
I have no preference on a BDouble with a precision of 0 but would be easy enough to code with ceil
@Ben-Henshall Thanks for pointing that out. I already added a test that checks these cases:
func testDecimalExpansion()
{
let testValues = [
("0", "0.0", 0),
("0", "0.0", 1),
("0", "0.00", 2),
("0", "0.000", 3),
("12.345", "12.0", 0),
("12.345", "12.3", 1),
("12.345", "12.34", 2),
("12.345", "12.345", 3),
("12.345", "12.3450", 4),
("-0.00009", "0.0000", 4),
("-0.00009", "-0.00009", 5),
("-0.00009", "-0.000090", 6),
]
for (original, test, precision) in testValues
{
let result = BDouble(original)!.decimalExpansion(precisionAfterDecimalPoint: precision)
XCTAssert(result == test)
}
}
But it would be nice if you'd take a look at them, maybe you can find some more cases that were not covered previously.
I also figured that the decimal expansion should always contain a decimal point, so the decimal expansion of "42" with precision 0 would be "42.0"
@twodayslate How about a second function like "decimalExpansionWithRounding"?
The test cases provided in there all passed with the previous code, so we should add tests to test for the issue that we've fixed. These can be added to the list of test cases you have there @mkrd. they are an example of a value that emitted the incorrect string of "0.0" as the decimal expansion :
("-0.7162", "-0.7", 1),
("-0.7162", "-0.71", 2),
("-0.7162", "-0.716", 3),
("-0.7162", "-0.7162", 4),
("-0.7162", "-0.7162000000", 10),
It's super minor, but I'd also suggest changing the XCTAssert(result == test)
to XCTAssertEqual(result, test)
instead as the latter gives detailed information on each test that fails and shows the comparison rather than a generic "This test failed" message.
As for decimal expansion with a precision of 0 being "x.0" rather than "x", you're absolutely right. That makes much more sense than my suggestion.