Modernize, clean up, and update dependencies (v0.3.0)#2
Merged
Conversation
Cleanup, modernization, and safety pass over @yipe/dice. All 1085 existing tests still pass (1091 with new coverage); typecheck, lint, build, and npm pack all green. No change to probability math / numeric output. Tier 1 — safe cleanup: - Remove stale package-lock.json (project is Yarn 4) and unused devDeps (ts-node, tsconfig-paths). - Remove console.error calls from the parser so the library no longer writes to a consumer's console (defensive behavior preserved). - Fix typos and stale comments; correct invalid 'miss' OutcomeType references in DiceQuery JSDoc. Tier 2 — internal refactor (no behavior change): - Drop impossible "[number, number]" iterator branches and the matching `as any` casts; PMF always iterates [number, Bin]. - Deduplicate Bin clone/scale logic in PMF via cloneBin/scaleBin helpers. normalize() keeps direct division for bit-identical results. - Remove dead commented-out code (collapseIdentical). - Tighten internal `any`: typed Dice.privateData, MaxOfNode, and a typed view for the protected lastConfig getter. Tier 3 — API changes (minor breaking, see CHANGELOG): - Add DiceParseError (exported); parse() now throws it with .expression and .cause. Messages unchanged, so existing catches keep working. - PMF.toJSON() now returns a plain object (no double-encoding via JSON.stringify); add PMF.toJSONString() for the old string form. - Tighten DiceQuery.firstSuccessSplit() to OutcomeType | OutcomeType[]. - Add DiceQuery.stdev() alias to match PMF.stdev(). - Bump version 0.2.23 -> 0.3.0; add CHANGELOG.md and README error-handling example. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Upgraded every devDependency to its highest available version. Verified green: typecheck, lint, 1091 tests, build, npm pack, and example all pass; yarn npm audit reports no advisories. - @types/node ^24.13.2 -> ^26.0.1 - @typescript-eslint/* ^8.61.0 -> ^8.62.0 - @vitest/coverage-v8 ^4.1.8 -> ^4.1.9 - vitest ^4.1.8 -> ^4.1.9 - eslint ^9.0.0 -> ^10.6.0 Already at latest (unchanged): tsup 8.5.1, tsx 4.22.4, typescript 6.0.3 (typescript is additionally capped at <6.1.0 by typescript-eslint's peer range). Note: eslint 10 and vitest 4 require Node >=20 for development, but this only affects the dev toolchain. The published package's `engines` (>=18.17) is unchanged since the built output has no such requirement for consumers. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Performance pass using statistical identities. No change to the materialized `combined` distribution; all 1085 prior tests pass (1102 with new coverage), plus typecheck/lint/build green. Profiling showed the DiceQuery constructor's eager N-way convolution dominates cost (~17 ms for a heavy 4-attack expression), while the resulting mean/variance are essentially free to read. Two statistical optimizations: - mean()/variance()/stddev() now use moment additivity for independent attacks (E[ΣX]=ΣE[X], Var[ΣX]=ΣVar[X]), computed directly from the single PMFs. - `combined` is now built lazily (first access) rather than in the constructor. Together, a query used only for DPR/mean/variance never convolves: ~17 ms -> ~0.001 ms (~15000x) for the heavy expression. Values are bit-exact for already-normalized singles and otherwise differ by at most a few ULP (within the library's tolerances; verified max relative error ~1e-15 mean / ~5e-11 variance across all test expressions at 1-4 attacks). Also: - combinedWithAttribution() reuses `combined` when all singles already carry attribution (parser PMFs do) — bit-for-bit identical, skips a redundant convolution pass (~1.8x on the warm chart path). - Added PMF.hasAttribution() (O(1)); withAttribution() reuses it. Deliberately did NOT replace the convolution core: a fused/allocation-lean inner loop is ~1.5x faster but not bit-identical (floating-point addition isn't associative), which isn't worth silently shifting published numeric results. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Convolve now accumulates directly into each destination bin instead of building a temporary Bin per (a,b) term and merging it via mergeInto. This removes one object allocation and one merge pass per term. The probability channel accumulates in the same order, so P(damage) is bit-identical. Only the per-label count/attr provenance sums re-associate ((x+y)+z vs x+(y+z) for shared outcome labels), shifting them by at most a few ULP. Verified across 2-16 attack folds: max Δp ~6e-16 (machine epsilon, does not grow with depth), max Δcount ~1e-14 at 16 folds — ~100x below the eps=1e-12 pruning threshold. All 1102 tests pass. ~1.6x faster on the chart/distribution path (heavy 4-attack convolveMany: 16.9ms -> 10.6ms). This supersedes the earlier decision to leave the convolution core untouched: deep-fold measurements showed the re-association error is negligible and never accumulates in the probability channel. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… pass A multi-agent audit (5 discovery agents + adversarial verifiers) surfaced and vetted these safe optimizations. Each was verified bit-for-bit identical (probabilities, counts, means, variance) across the full expression corpus; all 1102 tests pass. - PMF.fingerprint(): memoize the convolution cache-key fingerprint on the immutable PMF instead of re-summing every bin key on each convolve() call (incl. cache hits). ~36% faster warm cache-hit convolves. Same key string. - Dice.calculateHitDistribution(): hoist the 6 outcome distributions out of the per-face loop and read them directly instead of cloning O(faces*6) times. ~10% faster cold parse of wide-support expressions. - DiceQuery.toStackedChartData(): remove a dead O(N*L) precomputation pass whose result was discarded. ~2x faster. - Dice.toPMF(): iterate the internal face map directly (skip getFaceMap clone) and use for-in for the save-half check; multiplyDiceByDice() uses a Map. The adversarial pass also rejected several tempting-but-unsafe ideas: power() in convolveMany (breaks public provenance semantics), inlining DiceQuery in probExactlyK (drops normalization, shifts values), an LRUCache set() tweak (empirically broke a test), and lazy fingerprint eval (cache-key collisions). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…d reroll
A multi-agent correctness audit (domain experts + adversarial verifiers, each
building independent brute-force ground truth) surfaced these confirmed bugs.
All verified against brute-force enumeration; 1102 tests pass.
Moments (regressions from the earlier closed-form change):
- variance()/stddev() used the unstable E[X²]−E[X]² form, losing accuracy by
catastrophic cancellation with a large constant damage offset (1d6+1e8 gave
variance 2.0 instead of 2.9167). Now use the centered, additive-per-single
form (true value at every scale).
- mean()/variance() ignored an explicitly-passed `combined` that diverges from
convolve(singles), disagreeing with cdf/percentiles. Now fall back to the
provided combined for consistency.
Convolution:
- Non-raw convolve() divided by zero on a zero-mass operand (scaleMass(1/0)),
poisoning every bin to NaN and corrupting DiceQuery.combined.mean()/variance().
Now guarded (mGot !== 0); a zero-mass convolution stays mass 0, matching the
raw path.
Counting queries:
- probabilityOf(label) summed full bin.p for any bin merely containing the label,
badly over-counting when bins hold multiple outcomes (P(crit)=0.49 vs 0.05).
It now computes the correct Poisson-binomial marginal (= probAtLeastOne).
- probExactlyK([labels], k) array-path delegated to the buggy probabilityOf, so
it disagreed with the (correct) string-path; now both agree with the true
binomial.
- singleProb divides by the single's mass, so probAtLeastOne is mass-invariant.
- probAtLeastOne clamps to [0,1] (was returning 1.0000000002).
- firstSuccessWeights throws on pSpecial > pSuccess instead of returning
out-of-range probabilities.
Parser:
- hd6/hd20 (reroll-one shorthand) used a weighted union (deleteFace(1).combine),
giving P(1)=1/(2s−1); now uses reroll(1) for the correct P(1)=1/s². parse("2hd6")
now matches the builder's roll(2).d(6).reroll(1) exactly.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…tions Add tests/math-correctness.test.ts (35 tests): every expectation is checked against an independent brute-force enumeration or hand-derived closed form, covering convolution/mass invariants, moment additivity & stability, Poisson-binomial counting queries (string vs array paths), parser D&D mechanics (advantage/disadvantage/elven accuracy, crit, hd reroll, keep-highest pools, save-for-half), mixtures, and the firstSuccessWeights guard. Also pins single-attack conditional-stat correctness. Tighten the previously-loosened hd tests in string-damage.test.ts from toBeCloseTo(.,0) to exact now that the parser matches the builder's reroll(1). Document the remaining audit findings as KNOWN LIMITATIONS (code comments + CHANGELOG), pinned by tests, since they need API/architecture decisions: - parser crit probability is wrong with bonus to-hit dice (builder is correct); - multi-attack conditional stats (snapshot/damageStatsFrom/outcomeTotals) are size-biased expected counts, not probabilities; - parser save-for-half can mislabel outcomes on odd constant damage; - mixN/gate/branch build O(2^n) identifier strings on deep chains. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…s 2) Two more confirmed correctness bugs from the audit, verified against brute force. - Parser save-for-half mislabeled outcomes on odd/constant damage, e.g. "(d20 DC 15) * (3) save half" reported hit=0.7 / saveFail=0.3 instead of saveFail=0.7 / saveHalf=0.3. The "isSaveHalf" detection relied on a brittle heuristic (2×a-half-value appearing in the hit distribution) that false-negatived whenever full != 2*floor(full/2). It is now deterministic: a non-empty save distribution *is* the save-half signal. Damage/mean were always correct; only the saveHalf/saveFail/hit labels were wrong. - DiceQuery.snapshot() reported outcome "probabilities" aggregated from the combined PMF's count, which is an expected count for N>=2 attacks — so atLeastOneProbability could exceed 1 (e.g. 3.2 for 4 attacks). It now uses the correct Poisson-binomial marginals: atLeastOneProbability = probAtLeastOne, allProbability = probAtLeastK(label, N). Single-attack results are unchanged. (damageRange.avg remains a size-biased mean for N>=2; documented.) Not fixed (documented as known limitations, need an architectural change): - Parser crit probability with bonus to-hit dice (Bless): the crit mass depends on the d20 roll mode, which is encoded in face counts and destroyed once bonus dice are convolved in. A correct fix needs nat-20 tracking at the d20 level (what the builder AST already does). The builder API is correct; use it. Adds brute-force lock-down tests for both fixes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The Node 18 CI leg failed at Vitest startup: Vitest 4 imports `styleText` from node:util, which only exists in Node >= 20.12, so the test runner cannot start on Node 18. The published package itself still supports Node >= 18.17 (its ES2020 build uses no such APIs). Keep Node 18 in the matrix for typecheck/build/pack (verifying the package builds for its declared engines minimum), skip only the Vitest `Test` step there, and add Node 24 so three Node versions still run the test suite. Also set fail-fast: false so one leg failing no longer cancels the others. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Bug fix: PMF.compact() deleted sub-epsilon count/attr entries in place on bin objects that are shared by reference between PMFs, corrupting the source distribution. It now clones each surviving bin before pruning. Hardening: The parser rejects pathological expressions with a DiceParseError instead of hanging: dice over 1,000,000 faces, counts over 10,000, keep pools that would enumerate over 1,000,000 outcomes, and binary operations whose face-count product exceeds 100,000,000 (e.g. d100000 + d100000, previously a multi-second hang). Legitimate expressions, including d100000, are unaffected. Performance (behavior-preserving): Count queries compute each attack's success probability and the binomial DP once instead of per requested count; PMF.branch builds its mixture in a single pass; keepSumPMF uses integer DP state keys; computeMaxOfPMF is O(N) for large pools; reroll and binaryOp avoid repeated allocations. Also tightens Dice.outcomeData typing, adds a yarn format script, and folds the release notes into the 0.3.0 changelog entry. Adds characterization and parser guard test suites; full suite green, typecheck and lint clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
yipe
force-pushed
the
claude/elegant-dewdney-810505
branch
from
38b7db2 to
3970cb8
Compare
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
1 participant
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.
Cleanup, modernization, and safety pass over
@yipe/dice, plus a full dependency bump. No change to the probability math / numeric output — all 1085 original tests still pass (1091 with new coverage).Verification
typecheck✓ ·lint(ESLint 10) ✓ · 1091 tests ✓ ·build✓ ·npm pack✓ ·example✓ ·yarn npm audit→ no advisories.Commit 1 — Modernize & clean up (v0.3.0)
Tier 1 — safe cleanup (no behavior change)
package-lock.json(the project uses Yarn 4) and unused devDeps (ts-node,tsconfig-paths).console.errorcalls from the parser so the library no longer writes to a consumer's console (defensive behavior preserved).'miss'OutcomeTypereferences inDiceQueryJSDoc.Tier 2 — internal refactor (no behavior change)
[number, number]iterator branches and the matchingas anycasts (PMFalways iterates[number, Bin]).Binclone/scale logic inPMFviacloneBin/scaleBinhelpers (normalize()keeps direct division for bit-identical results).any(typedDice.privateData,MaxOfNode, a typed view for the protectedlastConfig).Tier 3 — API changes (minor breaking → v0.3.0)
DiceParseError(exported);parse()throws it with.expression/.cause. Messages unchanged, so existingtry/catchkeeps working; now narrowable viainstanceof.PMF.toJSON()now returns a plain object (fixesJSON.stringify(pmf)double-encoding); addedPMF.toJSONString()for the old string form.DiceQuery.firstSuccessSplit()toOutcomeType | OutcomeType[].DiceQuery.stdev()alias (matchesPMF.stdev()).0.2.23 → 0.3.0; addedCHANGELOG.mdand a README error-handling example.Commit 2 — Bump all dev dependencies to latest
@types/node@typescript-eslint/*@vitest/coverage-v8+vitesteslinttsup(8.5.1),tsx(4.22.4),typescript(6.0.3) were already at latest.Note
ESLint 10 and Vitest 4 require Node ≥20 for development only — the published package's
engines(>=18.17) is unchanged since the built output has no such requirement for consumers. The CI matrix still lists Node 18; sinceyarn testruns Vitest 4 (Node ≥20), the Node 18 leg may warn/fail (pre-existing — Vitest was already v4). Left CI/enginesuntouched as that's a maintainer support-policy decision.🤖 Generated with Claude Code