Repository navigation
Conversation
…ations
Implement the following GDA / IEEE 754 decimal operations that were
missing from apd:
- Context.Max, Min, MaxAbs, MinAbs: numeric and by-magnitude minimum
and maximum, with the specification's NaN and total-order tie-break
rules.
- Context.NextPlus, NextMinus, NextToward: the representable value
adjacent to an operand in a given direction.
- Context.LogB, ScaleB: the adjusted exponent, and scaling by a power
of ten.
- Context.CopySign: the magnitude of one operand with the sign of
another.
- Decimal.SameQuantum and Decimal.CmpTotalMag: companions to the
existing CmpTotal.
The operations follow the General Decimal Arithmetic specification,
including the condition flags they raise. New table-driven tests cover
signed zeros, subnormals near Etiny, values near Nmax, infinities, and
quiet and signaling NaNs; the expected values were cross-checked against
the Python standard library's decimal module.
Signed-off-by: S Afsan Rahmatullah <afsan.scorp@gmail.com>
mw5h
left a comment
There was a problem hiding this comment.
I did an adversarial review of this at adaaa00, with the official GDA .decTest vectors for all twelve operations (strict: Form, sign, coefficient, exponent, every flag, plus d==x and d==y aliasing), a 66k-case randomized cross-check against Python's decimal across 11 contexts and all 8 rounding modes, and hand-picked edge cases. Trial merge onto current master is clean and go test ./... passes.
What checks out. Non-aliased official vectors: max 222/222, min 211/211, maxmag 207/207, minmag 197/197, nextplus 103/103, nextminus 101/101, nexttoward 316/316 results, logb 121/121, copysign 57/57, samequantum 333/333; scaleb 141 pass with 8 flag-only misses and comparetotmag 246 pass with 18 NaN-payload misses, both pre-existing apd limitations (see below). Randomized Python cross-check: 0 mismatches for next_plus/next_minus/next_toward/logb/copy_sign/same_quantum over ~33k cases. Quiet-NaN-as-absent, sNaN handling, tie-breaks (±0, 1.0 vs 1.00, negatives), result rounding (prec 3: Max(1.23456, 1) = 1.23 inexact, rounded), nextplus(Nmax) = Inf, nextminus(Inf) = Nmax, nextplus(0) = 1E-Etiny, LogB/ScaleB special cases, the ScaleB bound (= Python's 2*(Emax+prec)), and traps via goError all behave as the spec says. That's a solid base. But two things are blocking.
1. Blocking: NextPlus/NextMinus/NextToward silently return garbage for BaseContext.WithPrecision(n)
nextUpPos/nextDownPos do nc.Add(d, x, &tiny) / nc.Sub(d, x, &tiny) and discard the returned (Condition, error). tiny = 1E(MinExponent-Precision); when that exponent is below the system limit of -100000 (true for every BaseContext.WithPrecision(n) context, which is the idiom in this repo's own example_test.go), or when the exponent gap exceeds 100000, upscale fails before d is written, so d is left untouched with res = 0, err = nil:
BaseContext.WithPrecision(9): NextPlus(1) = 0E0 want 1.00000001
NextToward(1, 2) = 0E0
NextMinus(1) = 0E0
NextMinus(0) = -1E-100008 (below the system MinExponent)
Emax/Emin ±60000, d pre-filled with junk: NextPlus(1E+59000) returns d unchanged, err = nil
Fix: implement the increment on the coefficient directly (round directed; pad to Precision digits or to Etiny; ±1; carry → exponent+1 / Nmax / Infinity; borrow → 99…9). That is O(precision), cannot fail, and also removes the add-tiny cost (near big exponents the current approach builds ~80k-digit BigInts, ~2ms per call vs ~1µs). At minimum, propagate the Add/Sub errors and reject contexts where etiny()-1 < MinExponent.
2. Blocking (one-line fix): CopySign and NextToward lose the sign when d aliases y
Both do d.Abs(x); d.Negative = y.Negative, which reads y after overwriting it when d == y. That violates the Context doc ("Arguments for any method can safely be used as both result and operand"), and 54 copysign plus 19 nexttoward official vectors fail in the d==y configuration:
d := -1; CopySign(d, 5, d) → 5 (cpsx004: want -5)
d := -7; NextToward(d, 7, d) → 6.99999999 (nextt102: want -7; sign lost, then Cmp≠0 so the next-minus path runs)
nextt551: got Infinity, want -Infinity
Fix: neg := y.Negative; d.Abs(x); d.Negative = neg.
3. Should fix
- Precision 0 is not rejected.
setNmaxyields coefficient10^0-1 = 0with exponentMaxExponent+1, so with&BaseContext:NextMinus(Infinity) = 0E100001,NextPlus(-Infinity) = -0E100001,NextPlus(1)is a 100,039-character number,NextPlus(0) = 1E-100000.Quo/ExpreturnerrZeroPrecisionStr; these should too. - ScaleB range handling.
if newExp > MaxExponent || newExp < MinExponent → NaN + InvalidOperation. With prec 9, Emax/Emin ±99000:ScaleB(1E+99000, 1500)→NaN invalid operation, although 1500 is well within the spec bound (198018). Spec/Python:Infinity Overflow Inexact Rounded. apd convention (Mul(1E+99000, 1E+1500)): overflow plus an "exponent out of range" error. Routing throughd.setExponent(...)+c.roundwould make SystemOverflow flow throughgoErrorlike everywhere else. - The flag-conformance claim doesn't hold under directed rounding.
Max/Min/MaxAbs/MinAbs/ScaleBinherit two pre-existingsetExponentbugs: overflow always goes to Infinity (spec: round-down, floor-for-positive, ceiling-for-negative go to Nmax), and subnormal rounding ignores the sign (e.g. prec 3, floor:Min(-1E-102, NaN)→ apd-0E-101, Python-1E-101). That's 773 result mismatches vs Python in the sweep, all in these ops. The PR clearly knows about both (thenextPluscomment and the Nmax clamp innextDownPoswork around them) but only inside next-. The durable fix is insetExponent(which benefits Add/Mul/etc. and makes the next- workarounds unnecessary); otherwise the description's "follow the GDA specification, including the condition flags" should be narrowed. - Wire the official vectors.
.decTestfiles exist for all twelve operations (3,188 vectors) and the repo convention is to run them viagda_test.go; the harness aliasesdto each operand and would have caught item 2.TestOrderingConditionsmasks toInvalidOperation|DivisionByZero, so Overflow/Underflow/Subnormal/Inexact/Rounded/Clamped are never asserted. Note the stock harness compares finite results withCmp, which is exponent-insensitive and too weak for max/min/copysign; and the 18ctmxNaN-payload vectors needGDAignoreentries since apd drops payloads.
4. Smaller
- API shape:
SameQuantumandCmpTotalMagare context-freeDecimalmethods, but the equally context-freeCopySignis aContextmethod that always returns(0, nil).func (d *Decimal) CopySign(x, y *Decimal) *Decimalnext toNeg/Abswould be more consistent, and this is hard to change after release. - Pre-existing and not caused here, for the record: overflow results lack
Roundedand clamped zeros gain a spuriousRounded(the 8 scaleb flag misses);CmpTotalMagcannot order NaN payloads becausesetStringdiscards them (the 18 comparetotmag misses; worth a doc note).CmpTotalMagalso allocates two coefficient copies where a sign-maskedcmpOrderwould do.
…DA vectors - NextPlus/NextMinus/NextToward now step the coefficient directly instead of adding a value below Etiny. This is O(precision), works for contexts such as BaseContext.WithPrecision(n) and large exponent ranges, and reports the system exponent limits through goError instead of leaving d untouched. Contexts with zero precision are rejected with errZeroPrecisionStr. - CopySign is now a Decimal method, alongside Neg and Abs. It and NextToward read y's sign before writing d, so d may alias y. - setExponent clamps overflow to Nmax under round-down, round-05up, floor (positive) and ceiling (negative), marks overflow as Rounded, and rounds subnormals using the value's sign. This fixes Max/Min/MaxAbs/MinAbs/ScaleB under directed rounding and removes the Nmax workaround from next-*. - ScaleB rounds through setExponent: results beyond the context overflow or underflow as the specification requires, and results beyond the system limits return the usual exponent out of range error. - CmpTotalMag compares magnitudes without copying its operands, and documents that NaN payloads are not ordered. - Run the official GDA vectors for all twelve operations through gda_test.go, comparing exponents as well as values for the operations that select or adjust an operand. The NaN payload cases of comparetotmag are ignored, as for comparetotal. TestOrderingConditions now checks every flag.
|
Thanks for such a thorough review. The test harness alone caught more than my own testing did. All of it is addressed in 1d9d7ff: 1. next-* with 2. Aliasing. 3. Should fix
4. Smaller
I also reran a randomized comparison against Python's |
Adds the following General Decimal Arithmetic (GDA) / IEEE 754 decimal operations that apd was missing:
Context.Max,Min,MaxAbs,MinAbs: numeric and by-magnitude minimum and maximum, with the specification's NaN and total-order tie-break rules.Context.NextPlus,NextMinus,NextToward: the representable value adjacent to an operand in a given direction.Context.LogB,ScaleB: the adjusted exponent, and scaling by a power of ten.Decimal.CopySign,Decimal.SameQuantumandDecimal.CmpTotalMag: context-free companions toNeg,AbsandCmpTotal.It also fixes two existing
setExponentbehaviours these operations depend on, which affect every operation that rounds:Rounded.Implementation notes worth a reviewer's attention:
Max/Mintreat a quiet NaN operand as absent (return the other operand); only a signaling NaN raisesInvalidOperation. Ties (numerically equal operands) are broken by total order, so e.g.Max(1.0, 1.00)returns1.0.BaseContext.WithPrecision(9)) returns the usual exponent out of range error.ScaleBrounds throughsetExponent, so a result beyond the context's range overflows or underflows. One beyond the system limits returns an exponent out of range error, asMuldoes.Testing:
.decTestvectors for all twelve operations run throughgda_test.go. That includes the result-aliases-operand checks, and finite results must match the exponent exactly for the operations that select or adjust an operand. The 18comparetotmagNaN-payload vectors are ignored, as forcomparetotal, because apd does not keep NaN payloads.BaseContext.WithPrecisioncontexts, and directed-rounding overflow.TestOrderingConditionschecks every flag.decimal(230k cases, 11 contexts, all 8 rounding modes) gives no result mismatches. The only flag difference is the existing extraRoundedon clamped zeros, which this PR does not change.