Gaussian math: inverse_cdf#429
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR adds standard-normal inverse CDF support to the fixed-point math library. It includes offline Python tooling for two-region coefficient generation and validation, new generated Move coefficient/evaluator modules, public ChangesInverse CDF feature
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant SD29x9
participant sd29x9_base
participant inverse_cdf_upper_raw
participant inverse_cdf_coefficients
Caller->>SD29x9: inverse_cdf(p)
SD29x9->>sd29x9_base: inverse_cdf(p)
sd29x9_base->>sd29x9_base: validate p in [0,1]
alt p >= 0.5
sd29x9_base->>inverse_cdf_upper_raw: p_raw
else p < 0.5
sd29x9_base->>inverse_cdf_upper_raw: 1 - p_raw
end
inverse_cdf_upper_raw->>inverse_cdf_coefficients: read central/tail coefficients
inverse_cdf_upper_raw->>inverse_cdf_upper_raw: evaluate rational and clamp
inverse_cdf_upper_raw-->>sd29x9_base: z_raw
sd29x9_base-->>Caller: wrapped result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #429 +/- ##
==========================================
+ Coverage 96.54% 96.62% +0.07%
==========================================
Files 32 34 +2
Lines 3328 3405 +77
Branches 782 794 +12
==========================================
+ Hits 3213 3290 +77
Misses 70 70
Partials 45 45
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/gaussian_codegen/Makefile (1)
30-47: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winSplit the expanded check target to satisfy checkmake.
The added inverse-CDF commands push
checkpast the configured target-body limit. Factor per-family checks and letcidepend oncheckinstead of duplicating the commands.Proposed fix
-check: ## Drift-check generated files vs the emitters (both exact; needs prettier) +check: check-cdf check-pdf check-inverse-cdf ## Drift-check generated files vs the emitters (both exact; needs prettier) + +check-cdf: $(PYTHON) -m gaussian_codegen.cdf.emit_coefficients --check $(PYTHON) -m gaussian_codegen.cdf.emit_test_vectors --check + +check-pdf: $(PYTHON) -m gaussian_codegen.pdf.emit_coefficients --check $(PYTHON) -m gaussian_codegen.pdf.emit_test_vectors --check + +check-inverse-cdf: $(PYTHON) -m gaussian_codegen.inverse_cdf.emit_coefficients --check $(PYTHON) -m gaussian_codegen.inverse_cdf.emit_test_vectors --check -ci: validate test ## Exactly what CI runs - $(PYTHON) -m gaussian_codegen.cdf.emit_coefficients --check - $(PYTHON) -m gaussian_codegen.cdf.emit_test_vectors --check - $(PYTHON) -m gaussian_codegen.pdf.emit_coefficients --check - $(PYTHON) -m gaussian_codegen.pdf.emit_test_vectors --check - $(PYTHON) -m gaussian_codegen.inverse_cdf.emit_coefficients --check - $(PYTHON) -m gaussian_codegen.inverse_cdf.emit_test_vectors --check +ci: validate test check ## Exactly what CI runs🤖 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/Makefile` around lines 30 - 47, The check target body is now too large, so split the repeated generator checks in Makefile into smaller per-family targets referenced by check. Move the cdf, pdf, and inverse_cdf --check commands into separate helper targets, then have check depend on them and update ci to depend on check instead of duplicating the same commands. Use the existing check, ci, and the gaussian_codegen.*.emit_* entry points to keep the structure easy to locate.Source: Linters/SAST tools
🧹 Nitpick comments (4)
math/fixed_point/sources/sd29x9/sd29x9_base.move (1)
319-319: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the existing
half_raw()helper instead ofcommon::scale!() / 2.
half_rawis already imported here (Line 6) and used for the same 0.5-probability threshold incdf()(Line 205). Recomputing it inline duplicates the magic value and diverges from the module's own convention.♻️ Proposed fix
- if (p_raw >= common::scale!() / 2) { + if (p_raw >= half_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/sources/sd29x9/sd29x9_base.move` at line 319, The threshold check in the sd29x9 fixed-point logic should reuse the existing half_raw() helper instead of recomputing common::scale!() / 2 inline. Update the branch in the same area as the probability rounding logic to call half_raw(), matching the convention already used by cdf() and keeping the threshold consistent and centralized.math/fixed_point/sources/ud30x9/ud30x9_base.move (1)
280-283: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
half_raw()instead ofcommon::scale!() / 2.Same duplication as in
sd29x9_base::inverse_cdf. Importinghalf_rawfromcdf(already used for this exact 0.5 threshold) avoids the repeated magic value.♻️ Proposed fix
-use openzeppelin_fp_math::cdf::cdf_nonneg_raw; +use openzeppelin_fp_math::cdf::{cdf_nonneg_raw, half_raw};- assert!(p_raw >= common::scale!() / 2, EProbabilityBelowHalf); // p ≥ 0.5 + assert!(p_raw >= half_raw(), EProbabilityBelowHalf); // p ≥ 0.5🤖 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/ud30x9/ud30x9_base.move` around lines 280 - 283, The `inverse_cdf` probability threshold check duplicates the 0.5 constant by using `common::scale!() / 2` directly. Update the `ud30x9_base::inverse_cdf` logic to reuse `half_raw()` from `cdf`, matching the existing pattern used in `sd29x9_base::inverse_cdf`, and keep the same `EProbabilityBelowHalf` validation with the shared helper.scripts/gaussian_codegen/inverse_cdf/emit_coefficients.py (1)
127-131: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRender doc-comment bounds from the emitted values.
These generated comments hard-code
975_000_000and6_300_000_000, while the constants come from.derive_output.json. If the fit changes, docs drift even though emitted constants stay correct.Proposed fix
-/// Central-vs-tail probability split at the raw `10^9` scale (`975_000_000`). +/// Central-vs-tail probability split at the raw `10^9` scale (`{fmt_u128(threshold_raw)}`). public(package) fun central_threshold_raw(): u128 {{ CENTRAL_THRESHOLD_RAW }} -/// Output saturation clamp `|z|` at the raw `10^9` scale (`6_300_000_000`). +/// Output saturation clamp `|z|` at the raw `10^9` scale (`{fmt_u128(max_z_raw)}`). public(package) fun max_z_raw(): u128 {{ MAX_Z_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 `@scripts/gaussian_codegen/inverse_cdf/emit_coefficients.py` around lines 127 - 131, The generated doc-comments for central_threshold_raw and max_z_raw are hard-coding numeric bounds instead of reflecting the emitted constants. Update emit_coefficients.py so the comments are rendered from the same CENTRAL_THRESHOLD_RAW and MAX_Z_RAW values used in the generated functions, keeping the documentation in sync with the values derived from .derive_output.json even if the fit changes.scripts/gaussian_codegen/inverse_cdf/emit_test_vectors.py (1)
130-162: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winEmit STYLEGUIDE section order in generated test-vector modules.
The generated Move currently emits constants before errors and places
#[test]functions without a// === Test-Only Helpers ===section. Update both renderers to emit// === Errors ===,// === Constants ===,// === Structs ===, then// === Test-Only Helpers ===.As per coding guidelines,
**/*.movefiles must follow STYLEGUIDE.md section ordering and testing standards. As per path instructions, new inverse-CDF.movemodules should be reviewed against STYLEGUIDE.md as the source of truth.Also applies to: 178-208
🤖 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/inverse_cdf/emit_test_vectors.py` around lines 130 - 162, The generated inverse-CDF test-vector module is missing the required STYLEGUIDE section ordering. Update the Move renderers in emit_test_vectors.py so they emit sections in the order `// === Errors ===`, `// === Constants ===`, `// === Structs ===`, then `// === Test-Only Helpers ===`, with the `#[test]` function placed under the test-only helpers section. Ensure the generator still renders the same `TestCase`, `TOLERANCE`, and `ETestCaseFailed` symbols, but reordered to match the standard used by the other renderer.Sources: Coding guidelines, Path instructions
🤖 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/tests/sd29x9_tests/inverse_cdf_test_vectors.move`:
- Around line 13-15: The test vector error string in the inverse_cdf generated
constants still embeds the implementation identifier TOLERANCE, so update the
source template in emit_test_vectors.py rather than the generated Move files.
Change the ETestCaseFailed message to plain English only, and make the same
template update for the UD30x9 inverse_cdf vector generation so both outputs
stay consistent.
---
Outside diff comments:
In `@scripts/gaussian_codegen/Makefile`:
- Around line 30-47: The check target body is now too large, so split the
repeated generator checks in Makefile into smaller per-family targets referenced
by check. Move the cdf, pdf, and inverse_cdf --check commands into separate
helper targets, then have check depend on them and update ci to depend on check
instead of duplicating the same commands. Use the existing check, ci, and the
gaussian_codegen.*.emit_* entry points to keep the structure easy to locate.
---
Nitpick comments:
In `@math/fixed_point/sources/sd29x9/sd29x9_base.move`:
- Line 319: The threshold check in the sd29x9 fixed-point logic should reuse the
existing half_raw() helper instead of recomputing common::scale!() / 2 inline.
Update the branch in the same area as the probability rounding logic to call
half_raw(), matching the convention already used by cdf() and keeping the
threshold consistent and centralized.
In `@math/fixed_point/sources/ud30x9/ud30x9_base.move`:
- Around line 280-283: The `inverse_cdf` probability threshold check duplicates
the 0.5 constant by using `common::scale!() / 2` directly. Update the
`ud30x9_base::inverse_cdf` logic to reuse `half_raw()` from `cdf`, matching the
existing pattern used in `sd29x9_base::inverse_cdf`, and keep the same
`EProbabilityBelowHalf` validation with the shared helper.
In `@scripts/gaussian_codegen/inverse_cdf/emit_coefficients.py`:
- Around line 127-131: The generated doc-comments for central_threshold_raw and
max_z_raw are hard-coding numeric bounds instead of reflecting the emitted
constants. Update emit_coefficients.py so the comments are rendered from the
same CENTRAL_THRESHOLD_RAW and MAX_Z_RAW values used in the generated functions,
keeping the documentation in sync with the values derived from
.derive_output.json even if the fit changes.
In `@scripts/gaussian_codegen/inverse_cdf/emit_test_vectors.py`:
- Around line 130-162: The generated inverse-CDF test-vector module is missing
the required STYLEGUIDE section ordering. Update the Move renderers in
emit_test_vectors.py so they emit sections in the order `// === Errors ===`, `//
=== Constants ===`, `// === Structs ===`, then `// === Test-Only Helpers ===`,
with the `#[test]` function placed under the test-only helpers section. Ensure
the generator still renders the same `TestCase`, `TOLERANCE`, and
`ETestCaseFailed` symbols, but reordered to match the standard used by the other
renderer.
🪄 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: 7afe7894-f1e2-4ff3-b1c1-5d602f187509
📒 Files selected for processing (25)
.github/workflows/test.ymlmath/fixed_point/README.mdmath/fixed_point/sources/internal/inverse_cdf.movemath/fixed_point/sources/internal/inverse_cdf_coefficients.movemath/fixed_point/sources/sd29x9/sd29x9.movemath/fixed_point/sources/sd29x9/sd29x9_base.movemath/fixed_point/sources/ud30x9/ud30x9.movemath/fixed_point/sources/ud30x9/ud30x9_base.movemath/fixed_point/tests/sd29x9_tests/inverse_cdf_test_vectors.movemath/fixed_point/tests/sd29x9_tests/inverse_cdf_tests.movemath/fixed_point/tests/ud30x9_tests/inverse_cdf_test_vectors.movemath/fixed_point/tests/ud30x9_tests/inverse_cdf_tests.movescripts/gaussian_codegen/Makefilescripts/gaussian_codegen/README.mdscripts/gaussian_codegen/inverse_cdf/.derive_output.jsonscripts/gaussian_codegen/inverse_cdf/__init__.pyscripts/gaussian_codegen/inverse_cdf/derive.pyscripts/gaussian_codegen/inverse_cdf/emit_coefficients.pyscripts/gaussian_codegen/inverse_cdf/emit_test_vectors.pyscripts/gaussian_codegen/inverse_cdf/validate.pyscripts/gaussian_codegen/pyproject.tomlscripts/gaussian_codegen/shared/arithmetic.pyscripts/gaussian_codegen/shared/constants.pyscripts/gaussian_codegen/shared/reference.pyscripts/gaussian_codegen/tests/test_inverse_cdf.py
Collaborator
|
Collaborator
Author
|
Good catch — will fix this in #431 rather than here:
|
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
3 participants
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.
Summary by CodeRabbit
New Features
Documentation
inverse_cdfAPI, accuracy/behavior guarantees, and code generation workflow for quantile support.Tests
Chores / CI