Repository navigation
Fix Decimal division by a value < 1 (#14) and NaN standardDeviation (#12) - #15
Merged
Merged
Conversation
Issue #14: `Decimal._divideOperationByDecimalImpl` computed the result precision as `d.precision - other.precision` and asserted it was non-negative. When the division by `other._n` is exact (e.g. `5 / 0.5`, where `d` is `1.0` with precision `0` and `other.precision` is `1`) the value is negative, throwing an `assert`/`ArgumentError`. It now uses `_multiplyOperationByPrecisionShift`, which handles both directions of the shift. Issue #12: `Statistics.compute` and `StandardDeviationComputerNum` computed the variance as `squaresSum - sum²/length`, which suffers catastrophic cancellation: for a collection of equal `double` values the variance can be slightly negative, so `sqrt` returns `NaN` (7239 of 20000 swept values of a 7-element collection). Both now use Welford's online algorithm, which is numerically stable and returns exactly `0.0` for equal elements. The `_compute(sum, squaresSum, length)` fallback, used when only the sums are available, clamps a negative variance at `0` (the `NaN` of an empty set is preserved). `Statistics.operator +` merged the deviation from the `BigInt` sums, which are truncated for a non-integer collection, yielding a wrong result (`4.626895640200471` instead of `0.0` when merging two identical `double` collections). It now uses the parallel variance algorithm (Chan et al.) over the already computed deviations and means. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SeKd27KB9LvaHFDodiDzPC
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #15 +/- ##
==========================================
- Coverage 92.69% 92.68% -0.02%
==========================================
Files 16 16
Lines 6177 6204 +27
==========================================
+ Hits 5726 5750 +24
- Misses 451 454 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Found by differential fuzzing (Decimal vs double, DynamicInt vs BigInt, the three numeric extension families against each other and against reference implementations) plus targeted review. Each fix has a regression test that fails without it. `Decimal` modulo: `DynamicNumber.operator %` is documented as Euclidean and a whole `Decimal` delivers it (delegating to `DynamicInt`), but a fractional one used a truncated division, taking the sign of the dividend. So `-7 % 3` was `2` while `-7.5 % 3` was `-1.5`. 14203 of 30000 fuzzed pairs disagreed with `num.operator %`; now 0. `DynamicInt.digits`: the `toString()` fallback (values with 10+ digits) counted the `-` sign, while the fast path discarded it via `abs()`. `digits` sets the precision of the division remainder refinement, so `(-a) / b` and `-(a / b)` returned results of different precision in 18% of sampled pairs. `Statistics.compute`, three defects in the `BigInt` branch: - It truncated fractional values. The branch is selected automatically when `max > (maxSafeIntSqrt ~/ length)`, so an ordinary `List<double>` of values above ~31.6M was silently truncated: `[100000000.5, 100000001.25, 100000002.75].statistics.sum` gave `300000003` instead of `300000004.5`. `useBigIntToCompute: true` did it at any magnitude. - The overflow guard only checked `max`, so a collection of large negative ints overflowed the `int` squares sum, yielding a negative `squaresSum` and `squaresMean`. - `BigInt.toInt()` clamped `sum`/`squaresSum` to the maximum `int` - the very overflow the branch exists to avoid. `UnitLength`: `mi` to `mm`/`mic`/`nm` used a mile of 1609 m instead of the exact 1609.344 m, a 2.1e-4 relative error where every other mile conversion is at ~6 significant digits. The reverse factors are exact, so round trips through a mile lost 0.02%. CSV: values were quoted when they contained the separator but an inner `"` was never escaped, so `both, "x"` was written as `"both, "x""` and read back as 2 corrupted fields. Column names were never quoted, so a name containing the separator made the header declare more columns than the rows carry. Both now follow RFC 4180. Also verified clean (no defects found): combinations, primes, the num/double/int extension families, and Bayes net inference. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SeKd27KB9LvaHFDodiDzPC
Two `int` literals added by the audit tests exceed 2^53, which `dart2js`
rejects outright ("can't be represented exactly in JavaScript"), failing
the whole file to compile and taking `statistics_base_test.dart` and
`statistics_dynamic_int_test.dart` out of the Chrome run.
- `statistics_base_test.dart`: compare against
`squaresSumBigInt.toInt()` (the clamped value on the running platform)
instead of the hard-coded int64 maximum. This also makes the assertion
correct on the web, where the clamp is at 2^53, not 2^63.
- `statistics_dynamic_int_test.dart`: build the large `int` at runtime.
The values beyond the platform `int` range are already covered by the
`DynamicInt.parse` cases in the test above.
Verified with `dart test --platform chrome`: 352 tests pass.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SeKd27KB9LvaHFDodiDzPC
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #14 and #12. Both were still reproducible on
master.Issue #14 —
Decimaldivision by a value < 1_divideOperationByDecimalImplcomputedprecision2 = d.precision - other.precisionand asserted it was>= 0. When the division byother._ncomes out exact,d.precisionis smaller thanother.precision:5 / 0.5givesd = 1.0(precision0) againstother.precision = 1, soprecision2 = -1andDecimal._computeScalethrowsArgumentError: Invalid precision: -1.Multiplying by
other._scale(a power of 10) is a precision shift, and_multiplyOperationByPrecisionShiftalready handles a negative shift (multiplies_nby10^|p|at precision0). This is the reporter's workaround, routed through the existing helper so it also works for precisions above the base-10 shortcut table.Issue #12 —
standardDeviationreturnsNaNThe
Iterableextension was fixed in v1.2.1, butStatistics.computeandStandardDeviationComputerNumstill usedsqrt((squaresSum - sum²/length)/length). That subtraction cancels catastrophically: for equaldoublevalues the variance lands slightly negative andsqrtreturnsNaN. Sweeping 7-element collections ofi/100foriin1..20000, 7239 producedNaNon the Dart VM — the iOS reports were this bug, not a platform quirk.Both now use Welford's online algorithm. It returns exactly
0.0for equal elements, and also fixes a silent wrong answer the old formula gave for large values ([1e8, 1e8+1, 1e8+2]returned0.0instead of0.8164965809277). It reproduces every existing expected value bit-for-bit.The
_compute(sum, squaresSum, length)fallback — used when only the sums are available, e.g. the publicStatisticsconstructor — can't do better than clamping a negative variance at0; the empty-setNaNis preserved, as existing tests require.Adjacent bug found in the same function
Statistics.operator +merged deviations fromsumBigInt/squaresSumBigInt, which are truncated for adoublecollection:[94.97 × 7]statistics gave4.626895640200471instead of0.0[10.5, 20.5, 30.5] + [100.5, 200.5]gave71.486instead of71.386It now uses the parallel variance algorithm (Chan et al.) over the already-computed deviations and means — an exact match with the direct computation, and unchanged for the integer cases the existing tests cover.
Tests
12 new tests (335 total, all passing;
dart analyzeclean). Each new test was verified to fail against the unfixed code: 2 fail without the decimal fix, 7 without the statistics fix.Coverage: both reported datasets; sweeps across lengths 2/3/7/10/33 × 2000 values; the large-value cancellation case; a 24×24 division matrix checked against
doubledivision; the negative and decimal-dividend division paths; merge-vs-direct equivalence; and pinned known values plus the empty/single-element edge cases so the fix can't drift.CHANGELOG.mdandpubspec.yamlbumped to1.2.2.🤖 Generated with Claude Code
https://claude.ai/code/session_01SeKd27KB9LvaHFDodiDzPC