Skip to content

Gaussian math: cdf and pdf improvements (Step-1)#431

Merged
immrsd merged 9 commits into
mainfrom
improve-cdf-pdf
Jul 3, 2026
Merged

Gaussian math: cdf and pdf improvements (Step-1)#431
immrsd merged 9 commits into
mainfrom
improve-cdf-pdf

Conversation

@immrsd

@immrsd immrsd commented Jul 2, 2026

Copy link
Copy Markdown
Collaborator

TL;DR

  • cdf/pdf are now monotone between every pair of adjacent representable
    inputs
    — the previously documented 1-ULP far-tail inversions are gone.
    Fixed by raising the internal Horner accumulation scale from 10^18 to
    10^36: gas-neutral (the arithmetic already runs in u256), coefficients
    still fit u128, no public API change.
  • Saturation domains are clamped to the exact analytic rounding points:
    CDF 6.3 → 6.109410205 (the smallest z whose Φ rounds to 1 at the
    10^-9 output resolution), PDF 6.5 → 6.402729806 (where φ rounds to
    0). The cut-off is lossless, and the fit no longer spends approximation
    budget on the dead tail.
  • New exhaustive CI gates and in-VM tests enforce both properties — the old
    grid-based check stepped ~10^5 raw inputs at a time and could not see
    single-input inversions by construction.
  • A follow-up PR builds on this one: it re-derives the coefficients against
    a bit-exact model of the on-chain integer pipeline, raising the
    correctly-rounded rate at the same degree, gas, and API.

Problem

  1. 1-ULP tail inversions. At 10^18, per-step floor-truncation noise
    (~2×10⁻⁵ ULP) exceeds Φ's true per-step increment in the far tail, so
    neighboring inputs could invert (first at cdf(4.533726001) < cdf(4.533726000); ~6.4k cases, with analogous pdf up-blips). This was a
    documented caveat, and it breaks consumers that assume a monotone CDF —
    e.g. a future binary-search inverse_cdf.
  2. Over-wide domain. Φ already rounds to 1.000000000 for
    z ≥ 6.109410205, but saturation sat at 6.3 — so ~35M tail inputs
    returned 0.999999999 where the correctly-rounded answer is 1.0
    (PDF: ~97M inputs, 6.5 vs 6.402729806). Fitting across that flat tail
    also degraded the approximation everywhere else.

Changes

  • horner.move: the accumulation scale is now a per-call parameter of
    mul_wad / horner_eval!; cdf.move / pdf.move promote raw inputs by
    10^27 and evaluate at WAD = 10^36.
  • Codegen: domain bounds moved to the analytic points (verified against a
    100-dps mpmath oracle); coefficients regenerated on the clamped domain with
    the degrees pinned (CDF 9 / PDF 10) so a regeneration can never silently
    change them. The fitting method itself is unchanged in this PR.
  • New CI gates (shared/gates.py, run by make ci):
    • Neighbor monotonicity: scans every adjacent input pair across the
      at-risk tail (2.1B / 2.4B pairs) via a vectorized proxy with exact
      big-integer recheck — 0 inversions.
    • Overflow margin: the peak u256 Horner product is 246 / 248 bits; the
      gate enforces clearance below 2^256, since 10^36 leaves only ~10 / ~8
      bits and any future degree or domain change must re-prove it.
  • Move tests: neighbor-resolution monotonicity windows at the historically
    at-risk inputs plus peak-product no-abort probes — 568 tests, up from 558.
  • Docs: the monotonicity caveat is upgraded to a guarantee; domain,
    precision, and error-bound prose updated. The 5-ULP accuracy contract is
    unchanged (empirical worst case on the validation grid stays < 1 ULP).

Generated files (*_coefficients.move, *_test_vectors.move,
.derive_output.json) are emitter output, protected by the byte-for-byte
--check drift guard.

PR Checklist

  • Tests
  • Documentation
  • Changelog

Summary by CodeRabbit

  • New Features
    • Improved Gaussian CDF and PDF accuracy with analytically tightened effective input cutoffs and more consistent behavior across the supported range.
  • Bug Fixes
    • Strengthened fixed-point tail behavior so CDF is non-decreasing and PDF is non-increasing between adjacent representable inputs, with updated saturation at the new boundaries.
  • Documentation
    • Updated CDF/PDF (and inverse CDF corner) documentation to reflect the revised effective domains and saturation/rounding behavior.
  • Tests
    • Added CDF/PDF monotonicity regression tests and regenerated deterministic CDF/PDF oracle vectors to match the updated approximations.

@coderabbitai

coderabbitai Bot commented Jul 2, 2026

Copy link
Copy Markdown

Review Change Stack

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 41ea5c74-afee-42fb-8826-7fef4b51d64b

📥 Commits

Reviewing files that changed from the base of the PR and between 9f6d77c and 7f6e6d5.

📒 Files selected for processing (8)
  • math/fixed_point/README.md
  • math/fixed_point/sources/internal/horner.move
  • math/fixed_point/sources/internal/inverse_cdf.move
  • math/fixed_point/sources/sd29x9/sd29x9_base.move
  • math/fixed_point/sources/ud30x9/ud30x9_base.move
  • scripts/gaussian_codegen/README.md
  • scripts/gaussian_codegen/shared/arithmetic.py
  • scripts/gaussian_codegen/shared/constants.py
✅ Files skipped from review due to trivial changes (4)
  • math/fixed_point/sources/sd29x9/sd29x9_base.move
  • math/fixed_point/sources/ud30x9/ud30x9_base.move
  • math/fixed_point/README.md
  • scripts/gaussian_codegen/README.md
🚧 Files skipped from review as they are similar to previous changes (2)
  • scripts/gaussian_codegen/shared/arithmetic.py
  • math/fixed_point/sources/internal/horner.move

📝 Walkthrough

Walkthrough

This PR increases Gaussian CDF/PDF fixed-point evaluation precision to 10^36, tightens the saturation bounds to 6.109410205 and 6.402729806, regenerates coefficients and test vectors, adds tail monotonicity/overflow gates, and updates the related docs, scripts, and tests.

Changes

WAD rescaling of Gaussian CDF/PDF pipeline

Layer / File(s) Summary
Horner engine: caller-supplied WAD scale
math/fixed_point/sources/internal/horner.move, math/fixed_point/tests/gaussian_tests/horner_tests.move, math/fixed_point/sources/internal/inverse_cdf.move
mul_wad and horner_eval! now take explicit scale arguments, the Horner tests pass the new scale through their cases, and inverse-CDF evaluation passes its own WAD constant.
CDF internal evaluator and coefficients
math/fixed_point/sources/internal/cdf.move, math/fixed_point/sources/internal/cdf_coefficients.move
The internal CDF evaluator now promotes raw inputs into 10^36, evaluates Horner polynomials at explicit WAD scale, and uses regenerated coefficients plus a tighter saturation bound.
PDF internal evaluator and coefficients
math/fixed_point/sources/internal/pdf.move, math/fixed_point/sources/internal/pdf_coefficients.move
The internal PDF evaluator now uses the same 10^36 accumulation scale, and its regenerated coefficient table and saturation bound are updated to the new domain.
Public docs and behavior text
math/fixed_point/sources/sd29x9/sd29x9_base.move, math/fixed_point/sources/ud30x9/ud30x9_base.move, math/fixed_point/README.md, CHANGELOG.md
Documentation updates the stated saturation thresholds, monotonicity claims, and fixed-point wording for both families.
Move test vectors and boundary tests
math/fixed_point/tests/sd29x9_tests/*, math/fixed_point/tests/ud30x9_tests/*
CDF/PDF oracle vectors and saturation/monotonicity tests are regenerated and retuned around the new bounds.
New neighbor-monotonicity Move test modules
math/fixed_point/tests/gaussian_tests/cdf_monotonicity_tests.move, math/fixed_point/tests/gaussian_tests/pdf_monotonicity_tests.move
New Move tests scan adjacent raw inputs near the tail and saturation regions.
Shared codegen constants and arithmetic
scripts/gaussian_codegen/shared/constants.py, scripts/gaussian_codegen/shared/arithmetic.py, scripts/gaussian_codegen/shared/move_emit.py
Adds family-specific WAD scales, tighter domain bounds, scale-aware Horner helpers, and parameterized quantization.
Tail gate validators
scripts/gaussian_codegen/shared/gates.py
Adds neighbor-monotonicity and overflow-margin checks over the Gaussian tails.
CDF codegen pipeline
scripts/gaussian_codegen/cdf/*
Pins the shipped degree, emits at CDF_WAD, and validates with the new tail gates.
PDF codegen pipeline
scripts/gaussian_codegen/pdf/*
Pins the shipped degree, emits at PDF_WAD, and validates with the new tail gates.
Codegen tests and README
scripts/gaussian_codegen/tests/*, scripts/gaussian_codegen/README.md
Tests and README coverage are updated for the new scales, bounds, and validation flow.

Estimated code review effort: 4 (Complex) | ~75 minutes

Possibly related PRs

Suggested reviewers: bidzyyys, ericnordelo, 0xNeshi

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description is detailed and matches the template, but it is missing the required 'Resolves #...' issue reference. Add a 'Resolves #' line near the top, then keep the existing TL;DR, Problem, Changes, and checklist sections.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title is concise and matches the main change: improved Gaussian cdf/pdf behavior in Step-1.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch improve-cdf-pdf

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

@codecov

codecov Bot commented Jul 2, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.59%. Comparing base (2a6ba9c) to head (7f6e6d5).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #431      +/-   ##
==========================================
- Coverage   96.62%   96.59%   -0.03%     
==========================================
  Files          34       34              
  Lines        3405     3405              
  Branches      793      794       +1     
==========================================
- Hits         3290     3289       -1     
  Misses         70       70              
- Partials       45       46       +1     
Flag Coverage Δ
contracts/access 65.46% <ø> (ø)
contracts/allowance 52.40% <ø> (ø)
contracts/finance 26.07% <ø> (ø)
contracts/timelock 54.57% <ø> (ø)
contracts/utils 44.09% <ø> (ø)
math/core 86.89% <ø> (-0.08%) ⬇️
math/fixed_point 63.70% <100.00%> (ø)

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.

@coderabbitai

coderabbitai Bot commented Jul 2, 2026

Copy link
Copy Markdown

Caution

Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted.

Error details
{"name":"HttpError","status":500,"request":{"method":"PATCH","url":"https://api.github.com/repos/OpenZeppelin/contracts-sui/issues/comments/4864682813","headers":{"accept":"application/vnd.github.v3+json","user-agent":"octokit.js/0.0.0-development octokit-core.js/7.0.6 Node.js/24","content-type":"application/json; charset=utf-8"},"body":{"body":"<!-- This is an auto-generated comment: summarize by coderabbit.ai -->\n<!-- review_stack_entry_start -->\n\n[![Review Change Stack](https://storage.googleapis.com/coderabbit_public_assets/review-stack-in-coderabbit-ui.svg)](https://app.coderabbit.ai/change-stack/OpenZeppelin/contracts-sui/pull/431?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack)\n\n<!-- review_stack_entry_end -->\n<!-- This is an auto-generated comment: review in progress by coderabbit.ai -->\n\n> [!NOTE]\n> Currently processing new changes in this PR. This may take a few minutes, please wait...\n> \n> <details>\n> <summary>⚙️ Run configuration</summary>\n> \n> **Configuration used**: Path: .coderabbit.yaml\n> \n> **Review profile**: CHILL\n> \n> **Plan**: Pro\n> \n> **Run ID**: `5a7db493-6b00-4b26-831b-8fed4bef3faa`\n> \n> </details>\n> \n> <details>\n> <summary>📥 Commits</summary>\n> \n> Reviewing files that changed from the base of the PR and between 85448cefdad3e1359bdb74b5e575e7334f724455 and baf2a1ffc75e9bb47c762e9723436446e0ed517e.\n> \n> </details>\n> \n> <details>\n> <summary>📒 Files selected for processing (38)</summary>\n> \n> * `CHANGELOG.md`\n> * `math/fixed_point/README.md`\n> * `math/fixed_point/sources/internal/cdf.move`\n> * `math/fixed_point/sources/internal/cdf_coefficients.move`\n> * `math/fixed_point/sources/internal/horner.move`\n> * `math/fixed_point/sources/internal/pdf.move`\n> * `math/fixed_point/sources/internal/pdf_coefficients.move`\n> * `math/fixed_point/sources/sd29x9/sd29x9_base.move`\n> * `math/fixed_point/sources/ud30x9/ud30x9_base.move`\n> * `math/fixed_point/tests/gaussian_tests/cdf_monotonicity_tests.move`\n> * `math/fixed_point/tests/gaussian_tests/horner_tests.move`\n> * `math/fixed_point/tests/gaussian_tests/pdf_monotonicity_tests.move`\n> * `math/fixed_point/tests/sd29x9_tests/cdf_test_vectors.move`\n> * `math/fixed_point/tests/sd29x9_tests/cdf_tests.move`\n> * `math/fixed_point/tests/sd29x9_tests/pdf_test_vectors.move`\n> * `math/fixed_point/tests/sd29x9_tests/pdf_tests.move`\n> * `math/fixed_point/tests/ud30x9_tests/cdf_test_vectors.move`\n> * `math/fixed_point/tests/ud30x9_tests/cdf_tests.move`\n> * `math/fixed_point/tests/ud30x9_tests/pdf_test_vectors.move`\n> * `math/fixed_point/tests/ud30x9_tests/pdf_tests.move`\n> * `scripts/gaussian_codegen/README.md`\n> * `scripts/gaussian_codegen/cdf/.derive_output.json`\n> * `scripts/gaussian_codegen/cdf/derive.py`\n> * `scripts/gaussian_codegen/cdf/emit_coefficients.py`\n> * `scripts/gaussian_codegen/cdf/emit_test_vectors.py`\n> * `scripts/gaussian_codegen/cdf/validate.py`\n> * `scripts/gaussian_codegen/pdf/.derive_output.json`\n> * `scripts/gaussian_codegen/pdf/derive.py`\n> * `scripts/gaussian_codegen/pdf/emit_coefficients.py`\n> * `scripts/gaussian_codegen/pdf/emit_test_vectors.py`\n> * `scripts/gaussian_codegen/pdf/validate.py`\n> * `scripts/gaussian_codegen/shared/arithmetic.py`\n> * `scripts/gaussian_codegen/shared/constants.py`\n> * `scripts/gaussian_codegen/shared/gates.py`\n> * `scripts/gaussian_codegen/shared/move_emit.py`\n> * `scripts/gaussian_codegen/tests/test_emit.py`\n> * `scripts/gaussian_codegen/tests/test_shared.py`\n> * `scripts/gaussian_codegen/tests/test_validate_mirror.py`\n> \n> </details>\n> \n> ```ascii\n>  ______________________________________\n> < All those GPUs aren't just for show. >\n>  --------------------------------------\n>   \\\n>    \\   \\\n>         \\ /\\\n>         ( )\n>       .( o ).\n> ```\n\n<!-- end of auto-generated comment: review in progress by coderabbit.ai -->\n\n<!-- finishing_touch_checkbox_start -->\n\n<details>\n<summary>✨ Finishing Touches</summary>\n\n<details>\n<summary>📝 Generate docstrings</summary>\n\n- [ ] <!-- {\"checkboxId\": \"7962f53c-55bc-4827-bfbf-6a18da830691\"} --> Create stacked PR\n- [ ] <!-- {\"checkboxId\": \"3e1879ae-f29b-4d0d-8e06-d12b7ba33d98\"} --> Commit on current branch\n\n</details>\n<details>\n<summary>🧪 Generate unit tests (beta)</summary>\n\n- [ ] <!-- {\"checkboxId\": \"f47ac10b-58cc-4372-a567-0e02b2c3d479\", \"radioGroupId\": \"utg-output-choice-group-unknown_comment_id\"} -->   Create PR with unit tests\n- [ ] <!-- {\"checkboxId\": \"6ba7b810-9dad-11d1-80b4-00c04fd430c8\", \"radioGroupId\": \"utg-output-choice-group-unknown_comment_id\"} -->   Commit unit tests in branch `improve-cdf-pdf`\n\n</details>\n\n</details>\n\n<!-- finishing_touch_checkbox_end -->\n<!-- tips_start -->\n\n---\n\n\n\n\n<sub>Comment `@coderabbitai help` to get the list of available commands.</sub>\n\n<!-- tips_end -->"},"request":{"retryCount":3,"signal":{},"retries":3,"retryAfter":16}}}

@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: 5

Caution

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

⚠️ Outside diff range comments (3)
math/fixed_point/tests/sd29x9_tests/pdf_tests.move (1)

85-92: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Use pdf() here, not cdf().

This guard still exercises the CDF path and compares against ONE_RAW, so it never validates the PDF tail behavior you changed. Switch it to pdf() and bound it to PDF_0_RAW (or assert the zero cutoff directly) so the test covers the intended contract.

🐛 Proposed fix
-    probes.destroy!(|raw| assert!(fixed(raw).cdf().unwrap() <= ONE_RAW));
+    probes.destroy!(|raw| assert!(fixed(raw).pdf().unwrap() <= PDF_0_RAW));
🤖 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/sd29x9_tests/pdf_tests.move` around lines 85 - 92, The
guard test in max_z_raw_is_analytical_saturation_point is still exercising the
CDF path and checking ONE_RAW instead of validating the PDF tail behavior.
Update the test to call pdf() on the relevant sd29x9 value, and assert against
PDF_0_RAW or the zero cutoff directly so the contract matches the intended
saturation behavior. Keep the change localized to the existing pdf_tests.move
test and use the sd29x9::pdf symbol to verify the corrected path.
scripts/gaussian_codegen/pdf/emit_test_vectors.py (1)

97-102: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Stale saturation-bound reference in docstring.

The docstring still says not on a real-valued > 6.5 comparison, referencing the pre-PR saturation boundary. MAX_Z_RAW is now 6.402729806 (line 64); this example should match to avoid confusing future readers about the actual cutoff.

📝 Proposed fix
     Saturation mirrors the on-chain boundary exactly: it triggers when the
     *quantized* input `z_raw` meets or exceeds `MAX_Z_RAW` (the `>=` test in
-    `pdf.move::pdf_nonneg_raw`), not on a real-valued `> 6.5` comparison."""
+    `pdf.move::pdf_nonneg_raw`), not on a real-valued `> 6.402729806` comparison."""
🤖 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 `@scripts/gaussian_codegen/pdf/emit_test_vectors.py` around lines 97 - 102,
Update the docstring in expected_pdf_raw to remove the stale “> 6.5” saturation
wording and replace it with the current MAX_Z_RAW-based cutoff so it matches the
actual boundary used by pdf_nonneg_raw; keep the explanation aligned with the
quantized-input >= MAX_Z_RAW behavior and reference the existing MAX_Z_RAW
constant rather than a hardcoded numeric example.
scripts/gaussian_codegen/cdf/emit_test_vectors.py (1)

89-94: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Stale saturation-bound reference in docstring.

The docstring still says not on a real-valued > 6.3 comparison, referencing the old saturation boundary. With MAX_Z_RAW now 6.109410205 (line 62), this example value is outdated and could mislead future maintainers about the actual cutoff.

📝 Proposed fix
     Saturation mirrors the on-chain boundary exactly: it triggers when the
     *quantized* input `z_raw` meets or exceeds `MAX_Z_RAW` (the `>=` test in
-    `cdf.move::cdf_nonneg_raw`), not on a real-valued `> 6.3` comparison."""
+    `cdf.move::cdf_nonneg_raw`), not on a real-valued `> 6.109410205` comparison."""
🤖 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 `@scripts/gaussian_codegen/cdf/emit_test_vectors.py` around lines 89 - 94, The
docstring on expected_phi_raw still references the old real-valued saturation
cutoff and should be updated to match the current MAX_Z_RAW-based behavior. Edit
the documentation in emit_test_vectors.py so it describes saturation using the
quantized boundary handled by cdf_nonneg_raw and replaces the outdated “> 6.3”
example with the current cutoff value or a neutral reference to MAX_Z_RAW.
🧹 Nitpick comments (1)
scripts/gaussian_codegen/cdf/emit_coefficients.py (1)

56-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Hardcoded 10^36 in doc text could drift from constants.CDF_WAD.

Unlike the MAX_Z_RAW note at Line 98, which dynamically interpolates {fmt_u128(MAX_Z_RAW)}, this doc text hardcodes `10^36` as a literal string rather than deriving it from constants.CDF_WAD. If the WAD scale changes again (as it just did in this PR, from 10^18), this comment could silently go stale.

♻️ Proposed fix to interpolate the WAD value dynamically
     return f"""{banner}
 /// Numerator and denominator coefficients for the AAA-rational standard-normal
 /// CDF approximation on the central domain `[0, {constants.MAX_Z}]`. All values are
-/// sign-magnitude pairs at CDF WAD (`10^36`) scale, indexed in ascending power
+/// sign-magnitude pairs at CDF WAD (`{constants.CDF_WAD}`) scale, indexed in ascending power
 /// order (index 0 is the constant term).
🤖 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 `@scripts/gaussian_codegen/cdf/emit_coefficients.py` around lines 56 - 58, The
CDF docstring in emit_coefficients currently hardcodes the WAD scale as 10^36,
which can drift from constants.CDF_WAD if the scale changes again. Update the
comment near the central-domain CDF approximation to interpolate the value
dynamically using the existing formatting helper pattern used elsewhere (for
example, the MAX_Z_RAW note), so the documentation always reflects
constants.CDF_WAD.
🤖 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 `@math/fixed_point/sources/internal/pdf.move`:
- Around line 32-45: Clarify the doc comment in pdf.move so the “degree 10” note
refers to the PDF approximation, not the CDF. Update the wording around
WAD/WAD_PER_RAW to say the headroom is tighter than the PDF/Horner polynomial
used here (and optionally mention TARGET_DEGREE or the PDF coefficient set), so
it no longer suggests the CDF is degree 10. Use the existing constant comments
near WAD and WAD_PER_RAW to locate the text.

In `@math/fixed_point/tests/gaussian_tests/cdf_monotonicity_tests.move`:
- Around line 45-49: The comment in monotone_just_below_saturation has the wrong
endpoint description for the assert_neighbor_monotone call. Update the note to
match the actual scanned range for assert_neighbor_monotone(6_108_000_000,
WINDOW), using the correct raw-input end value and keeping the explanation
aligned with the WINDOW-based span in cdf_monotonicity_tests.

In `@math/fixed_point/tests/gaussian_tests/pdf_monotonicity_tests.move`:
- Around line 46-50: The comment in monotone_just_below_saturation uses the
wrong end value for the scanned range, since
assert_neighbor_monotone(6_400_000_000, WINDOW) only covers inputs through
6_400_000_999, not 6.400999999. Update the inline note in
monotone_just_below_saturation to match the actual range implied by
assert_neighbor_monotone and WINDOW, and keep the wording consistent with the
analogous CDF test to avoid the same digit-grouping mistake.

In `@scripts/gaussian_codegen/shared/gates.py`:
- Around line 130-134: The overflow margin check in the sampling loop is not a
proof, since `range(0, max_z_raw, step)` only probes a coarse subset and
`horner_peak_product` may peak between samples. Replace the current
sampling-based gate in this block with an exhaustive or analytically bounded
check, such as an interval/derivative-based bound over the full domain, and only
use the result to enforce the `MIN_HEADROOM_BITS` margin if it is proven for all
inputs. Keep the logic centered around the existing `step`, `peak`, and
`horner_peak_product` flow so the location is easy to update.
- Around line 88-107: The monotonicity gate in the pair-checking loop is still
relying on the float64 proxy to skip some cases, so it is not formally provable.
Update the logic around `_proxy_output`, `_eval_int`, and the `bad | ambiguous`
filtering so every pair that cannot be ruled out by a coefficient-specific error
bound is exact-checked, or add a rigorous interval proof that excludes all
skipped pairs before trusting the proxy result.

---

Outside diff comments:
In `@math/fixed_point/tests/sd29x9_tests/pdf_tests.move`:
- Around line 85-92: The guard test in max_z_raw_is_analytical_saturation_point
is still exercising the CDF path and checking ONE_RAW instead of validating the
PDF tail behavior. Update the test to call pdf() on the relevant sd29x9 value,
and assert against PDF_0_RAW or the zero cutoff directly so the contract matches
the intended saturation behavior. Keep the change localized to the existing
pdf_tests.move test and use the sd29x9::pdf symbol to verify the corrected path.

In `@scripts/gaussian_codegen/cdf/emit_test_vectors.py`:
- Around line 89-94: The docstring on expected_phi_raw still references the old
real-valued saturation cutoff and should be updated to match the current
MAX_Z_RAW-based behavior. Edit the documentation in emit_test_vectors.py so it
describes saturation using the quantized boundary handled by cdf_nonneg_raw and
replaces the outdated “> 6.3” example with the current cutoff value or a neutral
reference to MAX_Z_RAW.

In `@scripts/gaussian_codegen/pdf/emit_test_vectors.py`:
- Around line 97-102: Update the docstring in expected_pdf_raw to remove the
stale “> 6.5” saturation wording and replace it with the current MAX_Z_RAW-based
cutoff so it matches the actual boundary used by pdf_nonneg_raw; keep the
explanation aligned with the quantized-input >= MAX_Z_RAW behavior and reference
the existing MAX_Z_RAW constant rather than a hardcoded numeric example.

---

Nitpick comments:
In `@scripts/gaussian_codegen/cdf/emit_coefficients.py`:
- Around line 56-58: The CDF docstring in emit_coefficients currently hardcodes
the WAD scale as 10^36, which can drift from constants.CDF_WAD if the scale
changes again. Update the comment near the central-domain CDF approximation to
interpolate the value dynamically using the existing formatting helper pattern
used elsewhere (for example, the MAX_Z_RAW note), so the documentation always
reflects constants.CDF_WAD.
🪄 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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 5a7db493-6b00-4b26-831b-8fed4bef3faa

📥 Commits

Reviewing files that changed from the base of the PR and between 85448ce and baf2a1f.

📒 Files selected for processing (38)
  • CHANGELOG.md
  • 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/internal/pdf.move
  • math/fixed_point/sources/internal/pdf_coefficients.move
  • math/fixed_point/sources/sd29x9/sd29x9_base.move
  • math/fixed_point/sources/ud30x9/ud30x9_base.move
  • math/fixed_point/tests/gaussian_tests/cdf_monotonicity_tests.move
  • math/fixed_point/tests/gaussian_tests/horner_tests.move
  • math/fixed_point/tests/gaussian_tests/pdf_monotonicity_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/pdf_test_vectors.move
  • math/fixed_point/tests/sd29x9_tests/pdf_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/pdf_test_vectors.move
  • math/fixed_point/tests/ud30x9_tests/pdf_tests.move
  • scripts/gaussian_codegen/README.md
  • scripts/gaussian_codegen/cdf/.derive_output.json
  • 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/pdf/.derive_output.json
  • scripts/gaussian_codegen/pdf/derive.py
  • scripts/gaussian_codegen/pdf/emit_coefficients.py
  • scripts/gaussian_codegen/pdf/emit_test_vectors.py
  • scripts/gaussian_codegen/pdf/validate.py
  • scripts/gaussian_codegen/shared/arithmetic.py
  • scripts/gaussian_codegen/shared/constants.py
  • scripts/gaussian_codegen/shared/gates.py
  • scripts/gaussian_codegen/shared/move_emit.py
  • scripts/gaussian_codegen/tests/test_emit.py
  • scripts/gaussian_codegen/tests/test_shared.py
  • scripts/gaussian_codegen/tests/test_validate_mirror.py

Comment thread math/fixed_point/sources/internal/pdf.move
Comment thread math/fixed_point/tests/gaussian_tests/cdf_monotonicity_tests.move
Comment thread math/fixed_point/tests/gaussian_tests/pdf_monotonicity_tests.move
Comment thread scripts/gaussian_codegen/shared/gates.py
Comment thread scripts/gaussian_codegen/shared/gates.py
@bidzyyys

bidzyyys commented Jul 2, 2026

Copy link
Copy Markdown
Collaborator
  • horner.move:116mul_wad doc still says "the generic scale is 10^18", but the WAD_U256 (10^18) constant was removed and the module is now fully caller-scaled. Drop/reword so no reader assumes a 10^18 default remains.

@bidzyyys bidzyyys left a comment

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.

Great job @immrsd, please remember to resolve @coderabbitai comments.

Comment thread math/fixed_point/sources/internal/horner.move Outdated
@immrsd

immrsd commented Jul 3, 2026

Copy link
Copy Markdown
Collaborator Author

All remaining review follow-ups addressed:

  • @bidzyyys mul_wad doc (horner.move:116): fixed in 0409b03 — dropped the "generic scale is 10^18" clause; the module is fully caller-scaled now (10^18 survives only as the offline Python mirror's default, which isn't relevant to Move readers).
  • Stale saturation bounds in the {cdf,pdf}/emit_test_vectors.py docstrings (outside-diff): fixed in 0409b03 (> 6.109410205 / > 6.402729806).
  • Hardcoded 10^36 in the coefficient banners (nitpick): fixed in 5f32a61 on the stacked improve-cdf-pdf-step2 branch — the emitters now derive the exponent from constants.CDF_WAD/PDF_WAD; today's emitted output is byte-identical, and any future scale change regenerates correctly.
  • sd29x9_tests/pdf_tests.move "use pdf(), not cdf()" (outside-diff, Major): false positive — there is no cdf() call in either pdf test file on this branch (grep returns nothing). Lines 85–92 are saturation_at_sd29x9_extremes / max_z_raw_is_analytical_saturation_point, both already calling pdf(); the proposed diff references probes / fixed() / ONE_RAW, which are symbols from the analogous cdf test — the finding pattern-matched the wrong file.

Note: the Step-2 follow-up branch was renamed improve-cdf-pdf-pyimprove-cdf-pdf-step2 (repository rules block force-pushes, so rebasing it onto this PR's updated tip required a fresh branch; the old branch is deleted).

@immrsd immrsd requested a review from bidzyyys July 3, 2026 07:07

@bidzyyys bidzyyys left a comment

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.

LGTM

@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.

LGTM

@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: 1

🤖 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 `@scripts/gaussian_codegen/shared/constants.py`:
- Around line 99-104: The docstring for the inverse-CDF saturation constant is
stale and still claims it matches the CDF bound. Update the rationale near
INVERSE_CDF_MAX_Z in constants.py to reflect that CDF MAX_Z is now 6.109410205
while INVERSE_CDF_MAX_Z remains 6.3, and rewrite the explanation so it no longer
says the two bounds are equal at the corner. Keep the note focused on why
inverse_cdf still clamps to 6.3 and preserve the existing context around the
endpoint p = 1.
🪄 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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 41ea5c74-afee-42fb-8826-7fef4b51d64b

📥 Commits

Reviewing files that changed from the base of the PR and between 9f6d77c and 7f6e6d5.

📒 Files selected for processing (8)
  • math/fixed_point/README.md
  • math/fixed_point/sources/internal/horner.move
  • math/fixed_point/sources/internal/inverse_cdf.move
  • math/fixed_point/sources/sd29x9/sd29x9_base.move
  • math/fixed_point/sources/ud30x9/ud30x9_base.move
  • scripts/gaussian_codegen/README.md
  • scripts/gaussian_codegen/shared/arithmetic.py
  • scripts/gaussian_codegen/shared/constants.py
✅ Files skipped from review due to trivial changes (4)
  • math/fixed_point/sources/sd29x9/sd29x9_base.move
  • math/fixed_point/sources/ud30x9/ud30x9_base.move
  • math/fixed_point/README.md
  • scripts/gaussian_codegen/README.md
🚧 Files skipped from review as they are similar to previous changes (2)
  • scripts/gaussian_codegen/shared/arithmetic.py
  • math/fixed_point/sources/internal/horner.move

@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

Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.

Actionable comments posted: 1

🤖 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 `@scripts/gaussian_codegen/shared/constants.py`:
- Around line 99-104: The docstring for the inverse-CDF saturation constant is
stale and still claims it matches the CDF bound. Update the rationale near
INVERSE_CDF_MAX_Z in constants.py to reflect that CDF MAX_Z is now 6.109410205
while INVERSE_CDF_MAX_Z remains 6.3, and rewrite the explanation so it no longer
says the two bounds are equal at the corner. Keep the note focused on why
inverse_cdf still clamps to 6.3 and preserve the existing context around the
endpoint p = 1.
🪄 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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 41ea5c74-afee-42fb-8826-7fef4b51d64b

📥 Commits

Reviewing files that changed from the base of the PR and between 9f6d77c and 7f6e6d5.

📒 Files selected for processing (8)
  • math/fixed_point/README.md
  • math/fixed_point/sources/internal/horner.move
  • math/fixed_point/sources/internal/inverse_cdf.move
  • math/fixed_point/sources/sd29x9/sd29x9_base.move
  • math/fixed_point/sources/ud30x9/ud30x9_base.move
  • scripts/gaussian_codegen/README.md
  • scripts/gaussian_codegen/shared/arithmetic.py
  • scripts/gaussian_codegen/shared/constants.py
✅ Files skipped from review due to trivial changes (4)
  • math/fixed_point/sources/sd29x9/sd29x9_base.move
  • math/fixed_point/sources/ud30x9/ud30x9_base.move
  • math/fixed_point/README.md
  • scripts/gaussian_codegen/README.md
🚧 Files skipped from review as they are similar to previous changes (2)
  • scripts/gaussian_codegen/shared/arithmetic.py
  • math/fixed_point/sources/internal/horner.move
🛑 Comments failed to post (1)
scripts/gaussian_codegen/shared/constants.py (1)

99-104: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Update the stale CDF-bound rationale.

Line 100 through Line 102 still say the inverse-CDF clamp is equal to CDF MAX_Z, but CDF MAX_Z is now 6.109410205 while INVERSE_CDF_MAX_Z remains 6.3.

Suggested wording update
 """Output saturation clamp for the quantile, as an exact decimal string: the
-largest `|z|` `inverse_cdf` returns. Deliberately equal to the CDF domain bound
-`MAX_Z` so the two functions agree at the corner (`cdf(6.3)` saturates to `1`, so
-`inverse_cdf(1)` saturates to `6.3`). The deepest real value at `10^9` input
+largest `|z|` `inverse_cdf` returns. This endpoint clamp intentionally remains
+`6.3`; CDF now rounds to saturation before this bound, so `cdf(6.3)` is already
+`1` and `inverse_cdf(1)` returns `6.3`. The deepest real value at `10^9` input
 resolution is `Φ⁻¹(1 − 10⁻⁹) ≈ 5.998`, so `6.3` is only ever returned for the
 exact endpoint `p = 1`."""
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

"""Output saturation clamp for the quantile, as an exact decimal string: the
largest `|z|` `inverse_cdf` returns. This endpoint clamp intentionally remains
`6.3`; CDF now rounds to saturation before this bound, so `cdf(6.3)` is already
`1` and `inverse_cdf(1)` returns `6.3`. The deepest real value at `10^9` input
resolution is `Φ⁻¹(1 − 10⁻⁹) ≈ 5.998`, so `6.3` is only ever returned for the
exact endpoint `p = 1`."""
🧰 Tools
🪛 Ruff (0.15.20)

[warning] 103-103: String contains ambiguous (MINUS SIGN). Did you mean - (HYPHEN-MINUS)?

(RUF001)

🤖 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 `@scripts/gaussian_codegen/shared/constants.py` around lines 99 - 104, The
docstring for the inverse-CDF saturation constant is stale and still claims it
matches the CDF bound. Update the rationale near INVERSE_CDF_MAX_Z in
constants.py to reflect that CDF MAX_Z is now 6.109410205 while
INVERSE_CDF_MAX_Z remains 6.3, and rewrite the explanation so it no longer says
the two bounds are equal at the corner. Keep the note focused on why inverse_cdf
still clamps to 6.3 and preserve the existing context around the endpoint p = 1.

@immrsd immrsd merged commit bb2405c into main Jul 3, 2026
32 checks passed
@immrsd immrsd deleted the improve-cdf-pdf branch July 3, 2026 13:19
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.

3 participants