Skip to content

Gaussian math: cdf function#345

Open
immrsd wants to merge 36 commits into
mainfrom
fp/impl-cdf
Open

Gaussian math: cdf function#345
immrsd wants to merge 36 commits into
mainfrom
fp/impl-cdf

Conversation

@immrsd

@immrsd immrsd commented Jun 2, 2026

Copy link
Copy Markdown
Collaborator

What landed

cdf is defined as a regular function on both fixed-point types — sd29x9_base::cdf(z: SD29x9): SD29x9 and ud30x9_base::cdf(z: UD30x9): UD30x9, each returning the same type as its input. Both compute Φ(z) as a rational N(|z|)/D(|z|) (two degree-9 polynomials), evaluated via Horner on a sign-magnitude u256 accumulator at WAD (10¹⁸), then divided back to the type's 10⁹ scale in one rounding step. Coefficients were fit offline (AAA, via scipy+mpmath) and committed as Move constants. New: sources/internal/gaussian.move (algorithm + Horner helpers) and sources/internal/cdf_coefficients.move (generated). Pure function — no objects, clock, randomness, or state.

Reading order (~20 min)

  1. sd29x9_base::cdf and ud30x9_base::cdf — the public entry points; both delegate to the same helper.
  2. internal/gaussian.move — sign-magnitude u256 arithmetic, horner_eval! macro, and the self-contained cdf_nonneg_raw central-domain evaluator (handles saturation and Φ(0) shortcut internally).
  3. internal/cdf_coefficients.move — generated; skim only (trust comes from §5, not eyeballing).
  4. sd29x9_base.move private decompose + new is_negative — the only edits to existing code on the SD29x9 side.

Background: what Φ is, and why it can't be built like ln/exp

What cdf computes. Φ(z) is the standard-normal CDF: the probability that a
normally-distributed value lands ≤ z — the area under the bell curve left of z.
It underpins options pricing, risk models, and sampling. (pdf is the curve's
height; inverse_cdf/PPF is Φ run backwards.)

Why it's hard on-chain. Φ has no elementary formula — it's defined by an
integral with no closed form. The textbook ways to get it use exp(−z²/2) or the
error function erf, but Move has no floating point and no transcendentals,
only integer arithmetic on our fixed-point types.

Why it's unlike functions the package already has. ln, exp, sqrt,
pow each have a self-contained on-chain recipe: shrink the input to a small
range with an identity (e.g. ln(2^k·m) = k·ln2 + ln(m)), then run a few Newton
steps or a short series — the answer is computed straight from the input every
time. Φ has no such trick: no range-reduction identity, no series that's both
accurate and cheap on-chain. Doing it directly isn't impossible, just
impractical — slow and inaccurate in fixed point.

The article's idea (Evan Kim, "On-Chain Atomic Gaussian Math"). Don't compute
Φ — approximate the whole curve with a rational function N(z)/D(z) (one
polynomial divided by another). A good rational matches Φ to ~10⁻⁹ using only
multiplies and one divide — no exp, no erf. Finding the best coefficients is
heavy numerical work, so the work is split:

  • Offline (Python), once: an algorithm called AAA (Adaptive
    Antoulas–Anderson, 2018) is fed high-precision samples of Φ and automatically
    produces a near-optimal rational fit — curve-fitting that needs no hand-tuning.
    The coefficients are validated against a 100-digit reference and baked into the
    Move source as integer constants.
  • On-chain, every call: just evaluate the two polynomials (Horner's method)
    and divide. Tiny, deterministic, constant gas.

This is why the Python toolkit exists. AAA fitting, 100-digit reference
values, and error validation cannot run on-chain (no floats, no arbitrary
precision, far too expensive). On-chain we keep only the cheap half — evaluating
fixed coefficients. The same toolkit will later produce pdf and inverse_cdf.

Why this is the right call here:

  • No transcendentals → no new dependency, deterministic, cheap, constant gas.
  • Near-optimal at low degree → hits the 5×10⁻⁹ target at degree 9.
  • Even symmetry is free → fit only [0, 6.3], reflect via Φ(−z) = 1 − Φ(z).
  • Proven → matches a deployed reference; we re-derived our own coefficients
    (didn't copy theirs) and beat its accuracy (~7×10⁻¹⁰). Alternatives (erf,
    plain series, lookup tables) are rejected with reasons in 01-research.

sd29x9_base::cdf and ud30x9_base::cdf

sd29x9_base::cdf — the signed entry point

public fun cdf(z: SD29x9): SD29x9 {
    let Components { mag, neg } = decompose(z.unwrap());
    let phi = cdf_nonneg_raw(mag as u128);
    let raw = if (neg) {
        assert!(phi >= HALF_RAW, EInternalNegSubUnderflow);
        common::scale!() - phi      // reflect: 1 − Φ(|z|)
    } else {
        phi
    };
    sd29x9::wrap(raw, false)
}
  • decompose, not abs(). abs()/negate() abort on min(); cdf must
    be total. sd29x9_base::decompose (private, same module) is total and returns
    Components { neg, mag } which destructures directly — no accessor functions
    needed because Components is defined in this same module.
  • Saturation lives inside the helper. gaussian::cdf_nonneg_raw short-circuits
    to ONE_RAW for z_raw ≥ MAX_Z_RAW, so neither base module replicates the guard.
    For negative inputs, the sign-flip common::scale!() - phi turns the saturated
    upper-bound into 0.
  • Reflect by subtraction, asserted. common::scale!() - phi can't underflow
    because Φ(|z|) ≥ 0.5 — and that's checked live, guarding against future regressions.
  • Same-type return. The probability [0, 1] fits naturally in non-negative
    SD29x9; the package convention is "return the input type" (cf. abs(SD29x9): SD29x9).

ud30x9_base::cdf — the unsigned entry point

public fun cdf(z: UD30x9): UD30x9 {
    wrap(cdf_nonneg_raw(z.unwrap()))
}

One line. UD30x9 is inherently non-negative, so no decomposition, no sign-flip.
Both type paths route through the same gaussian::cdf_nonneg_raw, guaranteeing
bit-for-bit equivalence on positives.

cdf_nonneg_raw (L123)

if (mag == 0) return HALF_RAW;                       // Φ(0) bit-exact (0.5)
let z_wad = (mag as u256) * common::scale_u256!();   // 10^9 → 10^18 (WAD)
// bind coeff vectors to locals once, then:
let n = horner_eval!(z_signed, num.length(), |i| (num_mags[i], num_negs[i]));
let d = horner_eval!(z_signed, den.length(), |i| (den_mags[i], den_negs[i]));
assert!(!horner::is_neg(&n), EInternalNumNegative);                       // INV-8
assert!(!horner::is_neg(&d) && horner::mag(&d) > 0, EInternalDenNonPositive); // INV-7
let phi = horner::mul_div_nearest_u256(mag(&n), 10^9, mag(&d));          // N·10^9/D
if (phi > ONE_RAW) ONE_RAW else phi                                      // overshoot clamp
  • WAD internally, cast once. Running at 10¹⁸ preserves the ~3×10⁻⁹
    precision down to the 10⁹ output; the single divide is the only down-cast.
  • Φ(0) special case → exact 0.5, removes any sign-flip asymmetry at 0.
  • Bind coeff vectors once so the Horner loop indexes locals, not module
    constants (this is why accessors return whole vectors — deviation D1).
  • Sign asserts = integrity checks on the generated file: a corrupted
    coefficient that flips a sign on [0, 6.3] aborts here, before any consumer.
  • Overshoot clamp: nearest rounding can yield 10⁹+1 near z=6.3; clamp keeps
    it a valid probability.

Watch: internal/gaussian.move's private MAX_Z_RAW and
internal/cdf_coefficients.move's MAX_Z_WAD are two sources of truth for the
same 6.3 threshold (one at UD30x9 scale, one at WAD). Consistent today, worth
unifying to prevent drift.

internal/gaussian.move

  • SignedScaled256 { mag: u256, neg: bool } — Move has no signed ints, and
    intermediates exceed u128, so sign-magnitude over u256 is the simplest
    correct encoding (canonical zero = (0, false)). Abilities copy, drop only:
    not storable. Declared public (D2): Move 2024 forbids public(package) on
    struct decls; package-internality is enforced by the abilities + package-only
    constructors.
  • signed_add (L81) / signed_mul_wad (L99): textbook sign-magnitude.
    mul_wad is a raw (a*b)/WAD (not a checked mul_div) — safe only because
    magnitudes are bounded (see budgets below). Flag if you want a checked multiply.
  • mul_div_nearest_u256 (L117): round(a·b/d) half-up; inlined to avoid a
    math/core dependency (D6).
  • horner_eval! macro (L145): takes a coefficient-accessor lambda so pdf
    and inverse-CDF reuse it with their own tables, and so the caller can bind
    vectors to locals. Coefficients pulled in ascending power order. The
    empty-polynomial abort sits in a helper the macro calls (D3, macro scoping).

Budgets to sanity-check

  • Overflow (INV-10): peak acc·z1.4×10³⁸, peak N·10⁹1.4×10²⁹,
    u256::MAX1.16×10⁷⁷. ~39 orders of headroom — but the bound depends on
    |z| ≤ 6.3
    , which the saturation guard enforces. Break the guard, break the bound.
  • Error (INV-17): contract ≤ 5×10⁻⁹ (5 ULP). Committed coefficients:
    6.78×10⁻¹⁰ ≈ 1 ULP. ~7× headroom.

Trusting the coefficients (without reading the numbers)

Generated, so the question is "do I trust the pipeline":

  • derive.py (local, scipy+mpmath): sweeps AAA degree, picks the smallest
    meeting 5×10⁻⁹, normalizes D(0)=1, checks N≥0/D>0 and coeff·WAD < 2^128.
  • validate.py (the CI gate, scipy+numpy only): parses the committed
    .move file and re-runs the CDF in Python with integer math mirroring the
    Move code exactly
    (cdf_simulate), asserting ≤ 5 ULP vs scipy.stats.norm.cdf.

So CI never re-runs AAA — it just confirms the committed numbers still meet the
contract against an independent oracle. cdf_simulate doubles as an executable
spec of gaussian::cdf_nonneg_raw. Coefficients are re-derived by us (not lifted);
the AUTO-GENERATED banner names the source scripts.

Likely questions

  • Two scales? WAD internally preserves precision; cast to 10⁹ once at the end.
  • Why does SD29x9::cdf return SD29x9 instead of UD30x9? Package convention: every function returns the type it was called on. Mirrors abs(SD29x9): SD29x9 (a non-negative SD29x9). Consumers needing UD30x9 can cast at the call site.
  • Why does cdf live in sd29x9_base.move / ud30x9_base.move instead of gaussian/? Same convention: per-type public API lives in the type's base module, alongside add, abs, pow, etc. internal/gaussian.move holds shared algorithm helpers that the base modules consume.
  • Why decompose not abs()? abs() aborts on min(); cdf must be total. decompose works directly on the raw u128 bit pattern, so it's defined for every SD29x9 including min().
  • Raw multiply overflow? No, magnitudes are bounded by the saturation guard inside cdf_nonneg_raw (not a runtime check — fair to ask for mul_div as defense-in-depth).
  • Always a valid probability? Yes: exact saturation + overshoot clamp + underflow assert.
  • State / randomness / objects? None. PTB-safe.
  • Locks us in for pdf/inverse_cdf? Opposite — internal/gaussian.move's Horner machinery is generic on purpose; both will reuse it, and each will add its own *_coefficients.move next to cdf_coefficients.move.

Summary by CodeRabbit

  • New Features
    • Added standard-normal cumulative distribution function (CDF) for fixed-point math types
    • SD29x9.cdf() and UD30x9.cdf() now available for Gaussian/normal CDF evaluation
    • Added SD29x9.is_negative() predicate
  • Documentation
    • Expanded guidance on how generated CDF coefficients and test vectors are produced and verified
  • Tests / CI
    • Added comprehensive CDF correctness and regression test vectors, plus offline generator validation in CI to guard against drift

@immrsd immrsd self-assigned this Jun 2, 2026
@coderabbitai

coderabbitai Bot commented Jun 2, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: ca726297-91d5-4552-9ac3-1c10c26b5d56

📥 Commits

Reviewing files that changed from the base of the PR and between 06cdef9 and d4f9f82.

📒 Files selected for processing (4)
  • math/README.md
  • math/fixed_point/README.md
  • math/fixed_point/sources/sd29x9/sd29x9_base.move
  • math/fixed_point/sources/ud30x9/ud30x9_base.move
✅ Files skipped from review due to trivial changes (2)
  • math/README.md
  • math/fixed_point/README.md
🚧 Files skipped from review as they are similar to previous changes (2)
  • math/fixed_point/sources/sd29x9/sd29x9_base.move
  • math/fixed_point/sources/ud30x9/ud30x9_base.move

📝 Walkthrough

Walkthrough

Adds a standard-normal CDF implementation for SD29x9 and UD30x9 fixed-point types using an offline-generated AAA-rational approximation. A Python pipeline (deriveemit_coefficientsemit_test_vectorsvalidate) produces committed Move coefficient tables and oracle test vectors; a new Move horner module provides WAD-scale sign-magnitude polynomial evaluation; a CI job validates drift.

Changes

Standard-Normal CDF Implementation

Layer / File(s) Summary
Codegen framework and shared infrastructure
scripts/gaussian_codegen/pyproject.toml, scripts/gaussian_codegen/Makefile, scripts/gaussian_codegen/README.md, scripts/gaussian_codegen/.gitignore, scripts/gaussian_codegen/shared/constants.py, scripts/gaussian_codegen/shared/move_emit.py, scripts/gaussian_codegen/shared/reference.py
Python package, Makefile targets (install, regen, validate, check, test, ci), README, and shared modules for WAD/scale constants, high-precision mpmath CDF oracle, and Move file emission/drift-guard helpers.
CDF derivation, emission, and validation pipeline
scripts/gaussian_codegen/cdf/derive.py, scripts/gaussian_codegen/cdf/emit_coefficients.py, scripts/gaussian_codegen/cdf/emit_test_vectors.py, scripts/gaussian_codegen/cdf/validate.py
derive.py fits AAA rational approximation to SciPy CDF and writes .derive_output.json; emit_coefficients.py quantizes to WAD u128 and renders cdf_coefficients.move; emit_test_vectors.py emits deterministic oracle test-vector modules for both fixed-point types; validate.py re-simulates committed integer arithmetic against SciPy reference.
Move Horner polynomial evaluator
math/fixed_point/sources/internal/horner.move, math/fixed_point/tests/gaussian_tests/horner_tests.move
Defines SignedScaled256 sign-magnitude type, WAD-scale add/add_coeff/mul_wad arithmetic, assert_polynomial_nonempty, and horner_eval! macro; validated by horner_tests.move covering arithmetic, canonicalization, and empty-polynomial abort.
CDF evaluator and auto-generated coefficients
math/fixed_point/sources/internal/cdf_coefficients.move, math/fixed_point/sources/internal/cdf.move
cdf_coefficients.move contains auto-generated numerator/denominator sign-magnitude constant vectors and max_z_raw; cdf.move implements cdf_nonneg_raw (saturation, zero case, Horner rational evaluation with integrity assertions and overshoot clamping) and a test-seam eval_rational_for_test.
SD29x9/UD30x9 public CDF API
math/fixed_point/sources/ud30x9/ud30x9_base.move, math/fixed_point/sources/ud30x9/ud30x9.move, math/fixed_point/sources/sd29x9/sd29x9_base.move, math/fixed_point/sources/sd29x9/sd29x9.move
ud30x9_base wraps cdf_nonneg_raw into UD30x9::cdf; sd29x9_base adds is_negative() and cdf() with sign-reflection (SCALE - phi) and underflow guard; both types re-export the new functions via their public facade modules.
Test helpers, is_negative tests, and CI job
math/fixed_point/tests/sd29x9_tests/helpers.move, math/fixed_point/tests/ud30x9_tests/helpers.move, math/fixed_point/tests/sd29x9_tests/is_negative_tests.move, .github/workflows/test.yml, math/README.md, math/fixed_point/README.md
Adds abs_diff/assert_within to both test helper modules; new is_negative_tests.move validates sign-bit detection; CI adds independent codegen job (Python 3.12, validate coefficients, drift-check test vectors, pytest); README documents the generated-code sections and CDF usage.
Auto-generated Move CDF test vectors
math/fixed_point/tests/sd29x9_tests/cdf_test_vectors.move, math/fixed_point/tests/ud30x9_tests/cdf_test_vectors.move
Oracle-computed TestCase tables emitted by emit_test_vectors.py; cdf_vectors_match_oracle() iterates all cases and asserts absolute error ≤ 5 ULP.
Move CDF correctness and safety tests
math/fixed_point/tests/sd29x9_tests/cdf_tests.move, math/fixed_point/tests/ud30x9_tests/cdf_tests.move
Validates well-known Φ values, saturation, unit-interval range, monotonicity, symmetry, determinism, coefficient-array integrity, overshoot clamping, defense-in-depth abort codes via eval_rational_for_test, and UD30x9/SD29x9 bit-exact cross-type equivalence.
Python codegen unit tests
scripts/gaussian_codegen/tests/test_emit.py, scripts/gaussian_codegen/tests/test_shared.py, scripts/gaussian_codegen/tests/test_validate_mirror.py
Tests for quantize/expected_phi_raw oracle behavior, shared constant/formatting/banner/drift-guard utilities, and full mirror arithmetic (add, mul_wad, mul_div_nearest, horner_eval, cdf_simulate) against committed coefficients.

Sequence Diagram(s)

sequenceDiagram
  rect rgba(100, 150, 200, 0.5)
    note over derive.py,cdf_coefficients.move: Offline generation (local only)
    derive.py->>scipy.stats.norm: sample reference CDF on training grid
    derive.py->>derive.py: AAA fit, select degree, enforce invariants
    derive.py-->>emit_coefficients.py: .derive_output.json
    emit_coefficients.py->>emit_coefficients.py: quantize coefficients to u128 WAD
    emit_coefficients.py-->>cdf_coefficients.move: write coefficient module
    emit_test_vectors.py->>mpmath: compute oracle Φ(z) values
    emit_test_vectors.py-->>cdf_test_vectors.move: write SD29x9/UD30x9 test-vector modules
  end
  rect rgba(100, 200, 150, 0.5)
    note over validate.py,scipy.stats.norm: CI validation
    validate.py->>cdf_coefficients.move: parse committed coefficients
    validate.py->>validate.py: simulate integer Horner eval
    validate.py->>scipy.stats.norm: compare worst absolute error
  end
  rect rgba(200, 150, 100, 0.5)
    note over SD29x9.cdf,horner_eval: On-chain evaluation
    SD29x9.cdf->>cdf_nonneg_raw: |z| raw
    cdf_nonneg_raw->>eval_rational: central-domain z
    eval_rational->>cdf_coefficients.move: load num/den vectors
    eval_rational->>horner_eval: evaluate polynomials
    eval_rational->>eval_rational: round ratio, clamp overshoot
    SD29x9.cdf->>SD29x9.cdf: reflect for negative z
  end
Loading

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~120 minutes

Suggested reviewers

  • 0xNeshi
  • bidzyyys
  • ericnordelo

Poem

🐇 Hoppity-hop through the Gaussian curve,
Coefficients fitted with polynomial verve!
From SciPy's AAA to Move's integer art,
Φ(z) is computed — quite a smart cart.
No floats on-chain, just Horner's neat trick,
The bunny signs off: this math makes me tick!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 29.63% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Gaussian math: cdf function' clearly and concisely describes the primary change: adding a CDF (cumulative distribution function) implementation to the Gaussian math module.
Description check ✅ Passed The PR description is comprehensive and well-structured, covering what was added, reading order, background context, technical details, and likely questions. It addresses all key aspects of the change including implementation strategy, design decisions, and trust/validation mechanisms.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fp/impl-cdf

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 8

🧹 Nitpick comments (1)
math/fixed_point/tests/gaussian_tests/horner_tests.move (1)

150-255: ⚡ Quick win

Add direct tests for mul_div_nearest_u256.

This suite covers the Horner primitives well, but it never hits the new final-rounding helper directly. A few cases for remainder < d/2, = d/2, > d/2, plus d == 0 abort behavior, would lock down the CDF cast-back boundary.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@math/fixed_point/tests/gaussian_tests/horner_tests.move` around lines 150 -
255, Add direct unit tests that call mul_div_nearest_u256 to cover
final-rounding behavior: create tests for remainder < d/2, remainder == d/2,
remainder > d/2 to verify rounding down/up/tie behavior and one test that
ensures d == 0 causes abort; place them alongside the horner tests (e.g., in
horner_tests.move) and use specific inputs that exercise the boundary
(numerator, denominator, and expected nearest result) so the helper
mul_div_nearest_u256, its tie-breaking behavior, and abort-on-zero-denominator
are explicitly validated.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In @.github/workflows/test.yml:
- Around line 141-142: The checkout step named "Checkout code" currently uses
"uses: actions/checkout@v4" without disabling credential persistence; update
that step in the codegen job to include a with block setting
persist-credentials: false so the checkout token is not written into git config
before running pip install -e ... and the subsequent python -m ... / pytest
commands.
- Around line 141-147: Update the workflow to pin the two GitHub Action usages
to immutable commit SHAs instead of major-version tags: replace
actions/checkout@v4 and actions/setup-python@v5 with their respective commit SHA
pins, and set persist-credentials: false on the actions/checkout step to prevent
the workflow token from being persisted to local git config while
repo-controlled commands run; ensure the checkout step still uses the same
inputs (e.g., ref if present) and that the pinned setup-python SHA matches the
current v5 release commit.

In `@math/fixed_point/codegen/cdf/derive.py`:
- Around line 209-219: The current check only ensures each coefficient × WAD
fits u128 (variables wad, max_mag, loop over num + den) but misses on-chain u256
intermediate overflows; update derive.py to simulate the on-chain Horner
evaluation and final numerator scaling using u256 bounds: for each x in the
validation domain, compute Horner using quantized num/den coefficients and after
every multiply or add check that the intermediate is <= 2**256 - 1, and also
check the final step N * 10**9 / D fits u256; if any intermediate exceeds 2**256
- 1, make validate.py fail (return nonzero) and print an explanatory error to
stderr so regenerated fits that passed per-coefficient u128 checks won’t
overflow Move u256 intermediates.

In `@math/fixed_point/codegen/cdf/emit_test_vectors.py`:
- Around line 1-19: The docstring and several string literals in
emit_test_vectors.py contain Unicode punctuation (e.g., the minus sign “−” and
the multiplication sign “×”) which Ruff flags; locate the docstring at the top
and the string occurrences around the reported ranges (lines ~1-19, ~137-145,
~190-198) and replace the Unicode minus (U+2212) with plain ASCII hyphen '-' and
the Unicode multiplication sign (U+00D7) with ASCII 'x' so all test descriptions
and messages use only ASCII punctuation.
- Around line 9-11: Update the saturation wording to match the implementation:
state that saturation occurs at the inclusive quantized boundary (i.e.,
quantize_z(z_str) >= MAX_Z_RAW or equivalently |z| >= 6.3), since |z| == 6.3 is
an endpoint case; ensure this description aligns with expected_phi_raw() and the
cdf saturation branch in cdf.move (the code handling the inclusive boundary).

In `@math/fixed_point/codegen/cdf/validate.py`:
- Around line 34-43: The validation currently uses constants.MAX_Z_RAW instead
of the committed value in args.coeffs; update the script to parse the committed
MAX_Z_RAW from args.coeffs (the parsed coefficients from cdf_coefficients.move)
and use that parsed max_z_raw for both cdf_simulate() and when building the
validation/sample grid (replace usages of constants.MAX_Z_RAW / MAX_Z_FLOAT
there), ensuring HALF_SCALE/SCALE-based computations derive from that parsed
bound so the gate covers the actual committed artifact.

In `@math/fixed_point/codegen/README.md`:
- Around line 73-78: The unlabeled fenced code blocks that show the pipeline
diagram (containing derive.py, .derive_output.json, emit_coefficients.py,
emit_test_vectors.py, cdf_coefficients.move, cdf_test_vectors.move and the
validate.py description) need explicit language tags to satisfy markdownlint
MD040; update those triple-backtick fences to use "```text" (and do the same for
the other similar fenced block further down around the other diagram/content) so
both blocks are marked as text.

In `@math/fixed_point/sources/internal/horner.move`:
- Around line 142-147: The half-up rounding check in mul_div_nearest_u256 uses
rem * 2 >= d which can overflow; replace that comparison with the overflow-free
form rem >= d - rem to avoid multiplying rem by 2. In function
mul_div_nearest_u256 (symbols: prod, quot, rem) change the if condition to use
rem >= d - rem and return quot + 1 when true, otherwise quot, leaving the rest
of the logic (prod = a * b; quot = prod / d; rem = prod - quot * d;) unchanged.

---

Nitpick comments:
In `@math/fixed_point/tests/gaussian_tests/horner_tests.move`:
- Around line 150-255: Add direct unit tests that call mul_div_nearest_u256 to
cover final-rounding behavior: create tests for remainder < d/2, remainder ==
d/2, remainder > d/2 to verify rounding down/up/tie behavior and one test that
ensures d == 0 causes abort; place them alongside the horner tests (e.g., in
horner_tests.move) and use specific inputs that exercise the boundary
(numerator, denominator, and expected nearest result) so the helper
mul_div_nearest_u256, its tie-breaking behavior, and abort-on-zero-denominator
are explicitly validated.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: df1f548b-3374-4d9b-8667-7af347d1f8b1

📥 Commits

Reviewing files that changed from the base of the PR and between 9cd0672 and 4629266.

📒 Files selected for processing (34)
  • .github/workflows/test.yml
  • math/fixed_point/README.md
  • math/fixed_point/codegen/.gitignore
  • math/fixed_point/codegen/Makefile
  • math/fixed_point/codegen/README.md
  • math/fixed_point/codegen/__init__.py
  • math/fixed_point/codegen/cdf/__init__.py
  • math/fixed_point/codegen/cdf/derive.py
  • math/fixed_point/codegen/cdf/emit_coefficients.py
  • math/fixed_point/codegen/cdf/emit_test_vectors.py
  • math/fixed_point/codegen/cdf/validate.py
  • math/fixed_point/codegen/pyproject.toml
  • math/fixed_point/codegen/shared/__init__.py
  • math/fixed_point/codegen/shared/constants.py
  • math/fixed_point/codegen/shared/move_emit.py
  • math/fixed_point/codegen/shared/reference.py
  • math/fixed_point/codegen/tests/test_emit.py
  • math/fixed_point/codegen/tests/test_shared.py
  • math/fixed_point/codegen/tests/test_validate_mirror.py
  • math/fixed_point/sources/internal/cdf.move
  • math/fixed_point/sources/internal/cdf_coefficients.move
  • math/fixed_point/sources/internal/horner.move
  • math/fixed_point/sources/sd29x9/sd29x9.move
  • math/fixed_point/sources/sd29x9/sd29x9_base.move
  • math/fixed_point/sources/ud30x9/ud30x9.move
  • math/fixed_point/sources/ud30x9/ud30x9_base.move
  • math/fixed_point/tests/gaussian_tests/horner_tests.move
  • math/fixed_point/tests/sd29x9_tests/cdf_test_vectors.move
  • math/fixed_point/tests/sd29x9_tests/cdf_tests.move
  • math/fixed_point/tests/sd29x9_tests/helpers.move
  • math/fixed_point/tests/sd29x9_tests/is_negative_tests.move
  • math/fixed_point/tests/ud30x9_tests/cdf_test_vectors.move
  • math/fixed_point/tests/ud30x9_tests/cdf_tests.move
  • math/fixed_point/tests/ud30x9_tests/helpers.move

Comment thread .github/workflows/test.yml
Comment thread .github/workflows/test.yml
Comment thread scripts/gaussian_codegen/cdf/derive.py
Comment thread scripts/gaussian_codegen/cdf/emit_test_vectors.py
Comment thread math/fixed_point/codegen/cdf/emit_test_vectors.py Outdated
Comment thread scripts/gaussian_codegen/cdf/validate.py
Comment thread math/fixed_point/codegen/README.md Outdated
Comment thread math/fixed_point/sources/internal/horner.move Outdated
@codecov

codecov Bot commented Jun 2, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.79518% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 80.09%. Comparing base (a21d7bb) to head (d4f9f82).

Files with missing lines Patch % Lines
math/fixed_point/sources/sd29x9/sd29x9_base.move 90.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #345      +/-   ##
==========================================
+ Coverage   78.90%   80.09%   +1.19%     
==========================================
  Files          23       26       +3     
  Lines        1782     1884     +102     
  Branches      640      659      +19     
==========================================
+ Hits         1406     1509     +103     
+ Misses        341      339       -2     
- Partials       35       36       +1     
Flag Coverage Δ
contracts/access 65.40% <ø> (ø)
contracts/utils 44.09% <ø> (ø)
math/fixed_point 60.15% <98.79%> (+3.56%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment thread math/fixed_point/sources/internal/horner.move Outdated
Comment thread math/fixed_point/sources/sd29x9/sd29x9_base.move
Comment thread math/fixed_point/sources/internal/horner.move Outdated
Comment thread math/fixed_point/sources/internal/horner.move Outdated
Comment thread math/fixed_point/sources/internal/horner.move Outdated
Comment thread math/fixed_point/sources/internal/horner.move Outdated
Comment thread math/fixed_point/sources/internal/horner.move Outdated
Comment thread math/fixed_point/sources/sd29x9/sd29x9_base.move Outdated
Comment thread math/fixed_point/sources/sd29x9/sd29x9_base.move Outdated
Comment thread math/fixed_point/sources/sd29x9/sd29x9_base.move Outdated
@0xNeshi

0xNeshi commented Jun 11, 2026

Copy link
Copy Markdown
Collaborator

Btw, excellent work! 🚀

Comment thread math/fixed_point/codegen/cdf/validate.py Outdated

public(package) fun is_neg(x: &SignedScaled256): bool { x.neg }

// === Arithmetic ===

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sanity check: run /move-quality skill


// === Constants ===

const WAD_U256: u256 = 1_000_000_000_000_000_000;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add short explanation what "WAD" means and about its purpose


// === Errors ===

/// Polynomial must have at least one coefficient

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: add missing dots to end of comments

@ericnordelo ericnordelo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation itself looks good, great job! but I think we should reconsider how we include and present it in the library. Right now the PR includes the full offline coefficient-generation pipeline alongside the runtime Move implementation. That creates a lot of noise in this repository. Most library users do not need to understand or maintain the AAA fitting process, they care about the public cdf API, its behavior, its error bounds, its saturation semantics, and whether the implementation is deterministic and safe to use on-chain.

I think we should separate the runtime library surface from the coefficient-generation tooling. The generated coefficients and the Move evaluator belong here, but the heavier research/codegen pipeline could live outside this package, possibly in a dedicated OpenZeppelin repository or another clearly separated tooling location. If we keep any generation or validation code here, it should be minimal and justified by reproducibility or CI needs.

We should also put more emphasis on documenting the useful properties of the CDF implementation:

  • maximum absolute error / ULP bound
  • valid input and output ranges
  • saturation behavior at |z| >= 6.3
  • exact Φ(0) = 0.5
  • symmetry for signed inputs
  • deterministic, pure, object-free execution
  • intended use cases and limitations

Those details should be easy to find in the README, public doc comments, examples, and eventually the docs site, because this is what will make users trust and integrate the function.

Finally, I think comments should be written (mostly) for users, and agents trying to understand the flow. Generated markers like AUTO-GENERATED — do not hand-edit may be useful, but the source should avoid over-explaining the offline pipeline in places where users are only trying to understand the runtime behavior. In this case for example users can’t modify that code, and we maintainers ofc should not modify these values manually.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
math/fixed_point/sources/internal/horner.move (1)

44-176: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Align module section layout to the required Move 2024 template.

This file uses custom section blocks (Constructors, Accessors, Arithmetic, Horner Evaluator) instead of the required canonical section order/titles for touched .move files. Please remap these functions under the mandated headings (notably // === Package Functions ===, plus required placeholders where applicable).

As per coding guidelines: “Follow Move 2024 conventions and section ordering exactly in all touched .move files: imports → // === Errors === → // === Constants === → // === Structs === → ... → // === Package Functions === ...”.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@math/fixed_point/sources/internal/horner.move` around lines 44 - 176, Replace
the custom section headers (Constructors, Accessors, Arithmetic, Horner
Evaluator) with the Move 2024 canonical section order. Reorganize all the
public(package) functions—zero, from_unsigned, from_coeff, mag, is_neg, add,
add_coeff, mul_wad, assert_polynomial_nonempty, and horner_eval—under the
required section headings following the Move 2024 template (Errors, Constants,
Structs, Package Functions, etc.). Ensure the section order and titles match the
mandatory convention exactly as specified in the coding guidelines.

Source: Coding guidelines

math/fixed_point/tests/gaussian_tests/horner_tests.move (1)

17-221: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Restructure test module headings to the repository’s canonical section names/order.

The test file uses custom heading labels (WAD-scale literals, Helpers, add, etc.) rather than the required section template for touched .move files. Please reorganize/retitle to the canonical sequence (including // === Constants === and // === Test-Only Helpers === where appropriate).

As per coding guidelines: “Follow Move 2024 conventions and section ordering exactly in all touched .move files…” and “in-module test code only under // === Test-Only Helpers ===”.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@math/fixed_point/tests/gaussian_tests/horner_tests.move` around lines 17 -
221, The test file uses custom section headings that don't follow the
repository's canonical naming convention. Replace the current headings with the
standard sections: use `// === Constants ===` for the WAD-scale constant
definitions, use `// === Test-Only Helpers ===` for the pos() and signed()
helper functions, and reorganize the remaining test sections under canonical
headings that reflect the functions being tested (such as `// === Tests: zero /
from_* / accessors ===`, `// === Tests: add ===`, etc.). Ensure all section
headers follow the Move 2024 conventions with the exact format `// ===
SectionName ===` and maintain proper ordering throughout the file.

Source: Coding guidelines

🧹 Nitpick comments (1)
math/fixed_point/sources/internal/horner.move (1)

132-155: ⚡ Quick win

Normalize public API docs to include explicit Parameters/Returns/Aborts blocks.

The API docs are descriptive, but the guide asks for explicit Parameters/Returns/Aborts sections where relevant. Please update these doc comments (and similar public/package APIs in this file) to that format for consistency.

As per coding guidelines: “Documentation: use /// for doc comments on public APIs and include Parameters/Returns and Aborts when relevant.”

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@math/fixed_point/sources/internal/horner.move` around lines 132 - 155,
Refactor the documentation blocks for the `horner_eval` macro and other
public/package APIs in the file to follow the explicit format with Parameters,
Returns, and Aborts sections. Reorganize the current descriptive content into
these standardized sections: move the parameter descriptions into a Parameters
block, specify what the function returns in a Returns block, and keep the abort
conditions in an explicit Aborts block. Ensure all public and package-scoped
functions follow this consistent documentation structure throughout the file.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@math/fixed_point/sources/internal/horner.move`:
- Around line 44-176: Replace the custom section headers (Constructors,
Accessors, Arithmetic, Horner Evaluator) with the Move 2024 canonical section
order. Reorganize all the public(package) functions—zero, from_unsigned,
from_coeff, mag, is_neg, add, add_coeff, mul_wad, assert_polynomial_nonempty,
and horner_eval—under the required section headings following the Move 2024
template (Errors, Constants, Structs, Package Functions, etc.). Ensure the
section order and titles match the mandatory convention exactly as specified in
the coding guidelines.

In `@math/fixed_point/tests/gaussian_tests/horner_tests.move`:
- Around line 17-221: The test file uses custom section headings that don't
follow the repository's canonical naming convention. Replace the current
headings with the standard sections: use `// === Constants ===` for the
WAD-scale constant definitions, use `// === Test-Only Helpers ===` for the pos()
and signed() helper functions, and reorganize the remaining test sections under
canonical headings that reflect the functions being tested (such as `// ===
Tests: zero / from_* / accessors ===`, `// === Tests: add ===`, etc.). Ensure
all section headers follow the Move 2024 conventions with the exact format `//
=== SectionName ===` and maintain proper ordering throughout the file.

---

Nitpick comments:
In `@math/fixed_point/sources/internal/horner.move`:
- Around line 132-155: Refactor the documentation blocks for the `horner_eval`
macro and other public/package APIs in the file to follow the explicit format
with Parameters, Returns, and Aborts sections. Reorganize the current
descriptive content into these standardized sections: move the parameter
descriptions into a Parameters block, specify what the function returns in a
Returns block, and keep the abort conditions in an explicit Aborts block. Ensure
all public and package-scoped functions follow this consistent documentation
structure throughout the file.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: c61513f1-0991-4d5e-98a4-1b4eb99555f5

📥 Commits

Reviewing files that changed from the base of the PR and between 4629266 and 06cdef9.

📒 Files selected for processing (32)
  • .github/workflows/test.yml
  • math/fixed_point/README.md
  • math/fixed_point/sources/internal/cdf.move
  • math/fixed_point/sources/internal/cdf_coefficients.move
  • math/fixed_point/sources/internal/horner.move
  • math/fixed_point/sources/sd29x9/sd29x9.move
  • math/fixed_point/sources/sd29x9/sd29x9_base.move
  • math/fixed_point/sources/ud30x9/ud30x9.move
  • math/fixed_point/sources/ud30x9/ud30x9_base.move
  • math/fixed_point/tests/gaussian_tests/horner_tests.move
  • math/fixed_point/tests/sd29x9_tests/cdf_test_vectors.move
  • math/fixed_point/tests/sd29x9_tests/cdf_tests.move
  • math/fixed_point/tests/sd29x9_tests/is_negative_tests.move
  • math/fixed_point/tests/ud30x9_tests/cdf_test_vectors.move
  • math/fixed_point/tests/ud30x9_tests/cdf_tests.move
  • scripts/gaussian_codegen/.gitignore
  • scripts/gaussian_codegen/Makefile
  • scripts/gaussian_codegen/README.md
  • scripts/gaussian_codegen/__init__.py
  • scripts/gaussian_codegen/cdf/__init__.py
  • scripts/gaussian_codegen/cdf/derive.py
  • scripts/gaussian_codegen/cdf/emit_coefficients.py
  • scripts/gaussian_codegen/cdf/emit_test_vectors.py
  • scripts/gaussian_codegen/cdf/validate.py
  • scripts/gaussian_codegen/pyproject.toml
  • scripts/gaussian_codegen/shared/__init__.py
  • scripts/gaussian_codegen/shared/constants.py
  • scripts/gaussian_codegen/shared/move_emit.py
  • scripts/gaussian_codegen/shared/reference.py
  • scripts/gaussian_codegen/tests/test_emit.py
  • scripts/gaussian_codegen/tests/test_shared.py
  • scripts/gaussian_codegen/tests/test_validate_mirror.py
💤 Files with no reviewable changes (3)
  • scripts/gaussian_codegen/.gitignore
  • scripts/gaussian_codegen/shared/constants.py
  • scripts/gaussian_codegen/shared/move_emit.py
✅ Files skipped from review due to trivial changes (7)
  • scripts/gaussian_codegen/pyproject.toml
  • math/fixed_point/README.md
  • scripts/gaussian_codegen/README.md
  • math/fixed_point/sources/internal/cdf_coefficients.move
  • math/fixed_point/tests/ud30x9_tests/cdf_test_vectors.move
  • math/fixed_point/tests/sd29x9_tests/cdf_test_vectors.move
  • math/fixed_point/sources/ud30x9/ud30x9.move
🚧 Files skipped from review as they are similar to previous changes (7)
  • .github/workflows/test.yml
  • math/fixed_point/sources/sd29x9/sd29x9.move
  • math/fixed_point/sources/ud30x9/ud30x9_base.move
  • math/fixed_point/tests/sd29x9_tests/is_negative_tests.move
  • math/fixed_point/sources/internal/cdf.move
  • math/fixed_point/tests/ud30x9_tests/cdf_tests.move
  • math/fixed_point/sources/sd29x9/sd29x9_base.move

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature]: cdf for fixed-point math library

3 participants