package modernization including better type stability #42
Conversation
Pull Request Test Coverage Report for Build 113
💛 - Coveralls |
Codecov Report
@@ Coverage Diff @@
## master #42 +/- ##
==========================================
- Coverage 95.28% 92.72% -2.56%
==========================================
Files 6 6
Lines 106 110 +4
==========================================
+ Hits 101 102 +1
- Misses 5 8 +3
Continue to review full report at Codecov.
|
| end | ||
| # Numerical value: (-1)^s * c * 10^q | ||
| struct Decimal <: AbstractFloat | ||
| s::Bool # sign can be 0 (+) or 1 (-) |
There was a problem hiding this comment.
Why don't we roll this into c ?
There was a problem hiding this comment.
I was actually wondering the same thing, I just didn't bother to change it. I only spent about half an hour on this PR, mostly for the sake of type stability. If the separate sign bit really is unnecessary, suggest somebody make another PR later to change it.
There was a problem hiding this comment.
Let me add another input. It appears that there is no standard for arbitrary precision decimal arithmetics (there is only IEEE 754-2008 standard introducing 32, 64, and 64-bit precisions - decimal32, decimal64 and decimal128, respectively, and these are already implemented in Julia package called DecFP). Since Decimals.jl package aims at implementing arbitrary precision decimal arithmetics, it may enjoy freedom from official standards. Good. Nonetheless, there is General Decimal Arithmetic Specification by IBM, which sort of extends the IEEE 754-2008 towards arbitrary precision. Perhaps it does not have the same status of official IEEE standards, but apparently, the now built-in Python package decimal is implementing it. I tried to sketch these relationships at the bottom of the front page (README.md).
Now, although I was not involved in the Decimals.jl package development (I only joined very recently) my perception of the package is that it might aim at the same goal as the Python's decimal. If this goal is shared, I think it would be wise to keep the arithmetic model as close as possible.
There was a problem hiding this comment.
I don't necessarily dispute any of that, but this PR was just a quick update mostly to get rid of the unnecessary type instability. This package clearly could use a lot of work, I'd like it if we could merge this to get in what improvement we can for now and work can continue.
There was a problem hiding this comment.
We should move this discusion into #44
| end | ||
| c, q = parameters(('.' in str) ? split(str, '.') : str) | ||
| normalize(Decimal((str[1] == '-') ? 1 : 0, c, q)) | ||
| normalize(Decimal((str[1] == '-'), c, q)) |
There was a problem hiding this comment.
| normalize(Decimal((str[1] == '-'), c, q)) | |
| normalize(Decimal(str[1] == '-', c, q)) |
| @@ -60,8 +62,10 @@ function Base.print(io::IO, x::Decimal) | |||
| end | |||
|
|
|||
| # Zero/one value | |||
| Base.zero(::Type{Decimal}) = Decimal(0,0,0) | |||
| Base.one(::Type{Decimal}) = Decimal(0,1,0) | |||
| Base.zero(::Type{Decimal}) = Decimal(false,0,0) | |||
There was a problem hiding this comment.
| Base.zero(::Type{Decimal}) = Decimal(false,0,0) | |
| Base.zero(::Type{Decimal}) = Decimal(false, 0, 0) |
| Base.zero(::Type{Decimal}) = Decimal(0,0,0) | ||
| Base.one(::Type{Decimal}) = Decimal(0,1,0) | ||
| Base.zero(::Type{Decimal}) = Decimal(false,0,0) | ||
| Base.one(::Type{Decimal}) = Decimal(false,1,0) |
There was a problem hiding this comment.
| Base.one(::Type{Decimal}) = Decimal(false,1,0) | |
| Base.one(::Type{Decimal}) = Decimal(false, 1, 0) |
| Base.signbit(x::Decimal) = x.s != 0 | ||
| Base.signbit(x::Decimal) = x.s | ||
|
|
||
| Base.show(io::IO, x::Decimal) = write(io, "decimal(\""*string(x)*"\")") |
There was a problem hiding this comment.
| Base.show(io::IO, x::Decimal) = write(io, "decimal(\""*string(x)*"\")") | |
| Base.show(io::IO, x::Decimal) = write(io, "decimal(\"$x\")") |
|
|
||
| # Multiplication | ||
| function *(x::Decimal, y::Decimal) | ||
| function Base.:(*)(x::Decimal, y::Decimal) | ||
| s = (x.s == y.s) ? 0 : 1 |
There was a problem hiding this comment.
| s = (x.s == y.s) ? 0 : 1 | |
| s = x.s != y.s |
| @@ -17,25 +17,25 @@ function +(x::Decimal, y::Decimal) | |||
| end | |||
|
|
|||
| # Negation | |||
| -(x::Decimal) = Decimal((x.s == 1) ? 0 : 1, x.c, x.q) | |||
| Base.:(-)(x::Decimal) = Decimal((x.s == 1) ? 0 : 1, x.c, x.q) | |||
There was a problem hiding this comment.
| Base.:(-)(x::Decimal) = Decimal((x.s == 1) ? 0 : 1, x.c, x.q) | |
| Base.:(-)(x::Decimal) = Decimal(!x.s, x.c, x.q) |
Seemed like this package needed a bit of an overhaul as it appears to be basically the default arbitrary precision decimal arithmetic package. It still needs a ton of work, but I think this is a good start. The most significant improvement is in type stability as the fields of
Decimalare now concrete types.At first I was going to make
Decimalparametric with a type parameter for the exponent, but ultimately I decided that the benefit if this was pretty dubious, so I set the exponent to simply be anInt. Because of this, these changes should be pretty much non-breaking.