feat(elreal): Phase E.1 -- math constants as 4-component static expansions#893
Conversation
…sions Phase E.1 of epic #873 (resolves #887). The foundation sub-issue of Phase E (#878): provides lazy elreal streams for the math constants (pi, e, ln2, ln10, ...) that exp/log/trig will need. Strategy: re-use the pre-computed multi-component expansions already maintained in include/sw/universal/number/qd/math/constants/qd_constants.hpp (precomputed offline by Scibuilders/Jack Poulson and Stillwater). The qd 4-component representations are exactly the depth-0..3 components of an elreal expansion, so we get ~212 bits of precision per constant with zero new offline computation. Mechanism: - New public static factory elreal::from_expansion(initializer_list) constructs an elreal from a pre-computed non-overlapping component expansion. Caller is responsible for the Shewchuk non-overlapping property; arithmetic and comparison on the result assume it holds. - Math constants header at include/sw/universal/number/elreal/math/ constants/elreal_constants.hpp wraps the qd component values in elreal_pi(), elreal_e(), elreal_ln2(), elreal_ln10(), elreal_sqrt2(), elreal_phi(), and a handful of pi multiples/fractions (pi/2, pi/4, 2pi). Naming convention matches ereal_pi() in the analogous ereal constants header. - Each constant function returns a fresh elreal with 4 materialised components and no generator. at(k>=4) returns 0.0 (implicit zero extension); deeper refinement is a future enhancement (BBP for pi, Taylor for e, Machin for ln2/ln10). Tests in elastic/elreal/math/constants/constants.cpp: - Each constant's leading double matches M_PI / M_E / M_LN2 / M_LN10 / M_SQRT2 from <cmath>. - 4-component sum matches a hard-coded 110-digit long-double reference within 1e-30 relative tolerance. - computed_depth() == 4; at(k>=4) returns the implicit zero. - elreal_pi() > elreal(M_PI) at the default budget (Phase D's lazy-real distinctness applied to the constants: the rational/correction bits in elreal_pi() make it observably greater than the IEEE-754 rounded M_PI). Same check for e (positive correction) and ln10 (negative correction). - elreal_pi() != elreal(M_PI) by the same distinctness mechanism. The 13 elreal binaries (this new test plus 12 prior) all PASS on gcc 13 and clang. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
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 (1)
📝 WalkthroughWalkthroughThis PR adds elreal mathematical constants implemented as precomputed 4-component expansions via a new Changeselreal Math Constants
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
elastic/elreal/math/constants/constants.cpp (1)
95-102: ⚡ Quick winAdd checks for the remaining newly added constants.
The suite currently skips direct validation for
elreal_pi_2,elreal_pi_4,elreal_2pi,elreal_lge,elreal_lg10, andelreal_sqrt3. Adding the same leading/component/depth assertions for these APIs will reduce regression risk.Possible test extension
static constexpr long double REF_SQRT2 = 1.414213562373095048801688724209698078569671875376948073176679737990732478462107038850L; +static constexpr long double REF_SQRT3 = 1.7320508075688772935274463415058723669428052538103806280558069794L; static constexpr long double REF_PHI = 1.618033988749894848204586834365638117720309179805762862135448622705260462818902449707L; @@ nrOfFailedTestCases += check_constant("sqrt2", elreal_sqrt2(), REF_SQRT2); + nrOfFailedTestCases += check_constant("sqrt3", elreal_sqrt3(), REF_SQRT3); nrOfFailedTestCases += check_constant("phi", elreal_phi(), REF_PHI); + nrOfFailedTestCases += check_constant("pi_2", elreal_pi_2(), REF_PI / 2.0L); + nrOfFailedTestCases += check_constant("pi_4", elreal_pi_4(), REF_PI / 4.0L); + nrOfFailedTestCases += check_constant("2pi", elreal_2pi(), REF_PI * 2.0L); @@ check_depth("ln10", elreal_ln10()); + check_depth("sqrt2", elreal_sqrt2()); + check_depth("sqrt3", elreal_sqrt3()); + check_depth("phi", elreal_phi()); + check_depth("pi_2", elreal_pi_2()); + check_depth("pi_4", elreal_pi_4()); + check_depth("2pi", elreal_2pi()); + check_depth("lge", elreal_lge()); + check_depth("lg10", elreal_lg10()); }Also applies to: 108-124
🤖 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 `@elastic/elreal/math/constants/constants.cpp` around lines 95 - 102, Add the missing validation calls for the new constants by mirroring the existing pattern that uses check_constant and the multi-component assertions: add checks for elreal_pi_2, elreal_pi_4, elreal_2pi, elreal_lge, elreal_lg10, and elreal_sqrt3 using check_constant (e.g., nrOfFailedTestCases += check_constant("pi_2", elreal_pi_2(), REF_PI_2) etc.), and ensure you include the same leading/component/depth assertions that are used for elreal_pi(), elreal_e(), elreal_ln2(), elreal_ln10(), elreal_sqrt2(), and elreal_phi() so these APIs get the identical validation coverage as the existing constants.
🤖 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.
Nitpick comments:
In `@elastic/elreal/math/constants/constants.cpp`:
- Around line 95-102: Add the missing validation calls for the new constants by
mirroring the existing pattern that uses check_constant and the multi-component
assertions: add checks for elreal_pi_2, elreal_pi_4, elreal_2pi, elreal_lge,
elreal_lg10, and elreal_sqrt3 using check_constant (e.g., nrOfFailedTestCases +=
check_constant("pi_2", elreal_pi_2(), REF_PI_2) etc.), and ensure you include
the same leading/component/depth assertions that are used for elreal_pi(),
elreal_e(), elreal_ln2(), elreal_ln10(), elreal_sqrt2(), and elreal_phi() so
these APIs get the identical validation coverage as the existing constants.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7909f6f0-a987-41cb-b7c1-e7ee001949eb
📒 Files selected for processing (5)
elastic/elreal/CMakeLists.txtelastic/elreal/math/constants/constants.cppinclude/sw/universal/number/elreal/elreal.hppinclude/sw/universal/number/elreal/elreal_impl.hppinclude/sw/universal/number/elreal/math/constants/elreal_constants.hpp
| // constants at depth-1 precision (~106 bits / ~32 decimal digits). Sourced | ||
| // from the standard mathematical literature -- the same digit strings used | ||
| // to populate qd_constants.hpp. | ||
| static constexpr long double REF_PI = 3.141592653589793238462643383279502884197169399375105820974944592307816406286208998628L; |
There was a problem hiding this comment.
long doubles are just 64/3.3 = 19.3 digits. These constants get rounded to just 19 digits and thus do not offer high-precision references to elreal
There was a problem hiding this comment.
Fixed in 33a0fff. You're right -- long double is aliased to double on MSVC, ARM, RISC-V 32, and most ARM64 platforms, so those reference constants were silently 53-bit on those targets and didn't validate the 212-bit contract at all. Dropped them entirely; new approach validates component-by-component against qd_constants.hpp (see top comment in the test file for the validation-strategy rationale).
| } | ||
|
|
||
| // Sum of all four components must match the reference within long-double | ||
| // precision tolerance (long double on most platforms is at least 64 bits; |
There was a problem hiding this comment.
there are no long doubles on MSVC, ARM, or RISC-V 32, all these platforms the long double is aliased to double, so this will fail on these platforms.
There was a problem hiding this comment.
Fixed in 33a0fff -- same root cause as the previous comment. The long-double reference path is gone.
| // --- Leading-double round-trip against std library values ---------- | ||
| { | ||
| if (elreal_pi().at(0) != M_PI) { | ||
| std::cerr << "FAIL: elreal_pi().at(0) != M_PI\n"; |
There was a problem hiding this comment.
don't use M_PI, M_E, etc. as on MSVC they need to be enabled. Use the stdlib versions std::constants
There was a problem hiding this comment.
Fixed in 33a0fff. Switched all M_PI / M_E / M_LN2 / M_LN10 / M_SQRT2 references to std::numbers::pi_v<double> / e_v<double> / ln2_v<double> / ln10_v<double> / sqrt2_v<double>. Portable across MSVC, libstdc++, libc++, ARM, RISC-V toolchains; no _USE_MATH_DEFINES dance.
| // the depth-1+ correction makes them observably different (this is the | ||
| // same lazy-real distinctness test from Phase D's compare.cpp). | ||
| { | ||
| if (elreal_pi() == elreal(M_PI)) { |
There was a problem hiding this comment.
again, don't use M_PI, use the stdlib constants
There was a problem hiding this comment.
Fixed in 33a0fff -- same as the previous comment, all M_PI uses are gone in favor of std::numbers.
| static constexpr long double REF_SQRT2 = 1.414213562373095048801688724209698078569671875376948073176679737990732478462107038850L; | ||
| static constexpr long double REF_PHI = 1.618033988749894848204586834365638117720309179805762862135448622705260462818902449707L; | ||
|
|
||
| static int check_constant(const char* name, sw::universal::elreal value, long double ref) { |
There was a problem hiding this comment.
this is not helpful. First of all, long double is on many platforms aliased to double, and what is the validation value of looking at the leading double of the expansion?
The validation should work on the basis of 100 or 200 digit references that we can generate off line with a multitude of tools. long double as marshalling type does not work.
There was a problem hiding this comment.
Fixed in 33a0fff. The validation strategy is now anchored on the qd_constants.hpp precomputation. E.1 literally re-uses qd_pi / qd_e / etc. (precomputed offline by Scibuilders/Stillwater, used in production for years), so component-by-component equality with qd is the exact validation of E.1's contract: did I correctly port those values? The new check_constant_vs_qd helper iterates components 0..3 and asserts exact double match. The component contract is therefore validated at full 212-bit precision, not at the 53-bit leading-double level. The depth-0-only check kept against std::numbers is now just one of several layers; the long-double layer is gone.
Address Ravenwater's substantive review feedback on PR #893: 1. `long double` is just `double` on MSVC, ARM, RISC-V 32, and most ARM64 platforms -- the previous `static constexpr long double REF_PI = ...` constants were actually only 53-bit double constants on those platforms and validated nothing past depth 0. Dropped them entirely. 2. `M_PI`, `M_E`, etc. are POSIX, not standard C++. They require _USE_MATH_DEFINES on MSVC. Replaced with `std::numbers::pi_v<double>`, `std::numbers::e_v<double>`, etc. (C++20, portable across MSVC, libstdc++, libc++, ARM, RISC-V toolchains). 3. Leading-double comparison only validated 53 bits, not the 212-bit contract. The proper validation is component-by-component against qd_constants.hpp -- elreal_constants.hpp literally re-uses the qd precomputation (Scibuilders/Stillwater offline values used in production for years), so component-by-component equality with qd_pi/qd_e/qd_ln2/... is the exact validation of the actual contract ("did E.1 correctly port the precomputed values?"). Added check_constant_vs_qd() that iterates components 0..3 and asserts exact match with the qd reference. 4. CodeRabbit nitpick: extended coverage from {pi, e, ln2, ln10, sqrt2, phi} to ALL the constants shipped in elreal_constants.hpp: pi_2, pi_4, 2pi, lge, lg10, sqrt3. Each gets the same component match, depth-4 structural check, and (where applicable) the std::numbers depth-0 round comparison. Verified PASS under gcc 13 and clang. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Review status after first pass (commit 33a0fff):
The validation strategy is now documented in a comment block at the top of the test file. New three-layer approach: (a) depth-0 vs std::numbers, (b) full 4-component match vs qd, (c) structural invariants. The long-double layer is gone. Verified PASS under gcc 13 and clang. |
Coverage Report for CI Build 26204585038Coverage decreased (-0.009%) to 84.22%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions8 previously-covered lines in 1 file lost coverage.
Coverage Stats
💛 - Coveralls |
github-project-automation
Bot
moved this from In progress
to Done
in Universal Number Library
2 participants
Summary
Phase E.1 of epic #873. Implements #887 (the foundation sub-issue under Phase E #878).
Provides lazy elreal streams for the math constants -- pi, e, ln2, ln10, sqrt2, phi, plus pi multiples/fractions. The exp/log family (#889) and the trig families (#891, #892) will lean on these, particularly pi for trig range reduction and ln2 for exp/log range reduction.
Mechanism
Re-use the qd precomputation. Universal already maintains 4-component multi-precision constants in
include/sw/universal/number/qd/math/constants/qd_constants.hpp(precomputed offline by Scibuilders/Jack Poulson and Stillwater). Those 4-component representations are exactly the depth-0..3 components of an elreal lazy stream, so this PR gets ~212 bits of precision per constant with zero new offline computation.New public factory
elreal::from_expansion(initializer_list)constructs an elreal from a pre-computed non-overlapping component expansion. The caller asserts the Shewchuk non-overlapping property (|c_{i+1}| <= ulp(c_i)/2); arithmetic and comparison on the result rely on it.Constants header at
include/sw/universal/number/elreal/math/constants/elreal_constants.hppwraps the qd component values inelreal_pi(),elreal_e(), etc. Naming follows the existingereal_pi()convention.No generator installed. Each constant returns a fresh elreal with 4 materialised components and
_generator = {}.at(k >= 4)returns the implicit-zero extension. Deeper refinement is a future enhancement (BBP for pi, Taylor for e, Machin for ln2/ln10).Test plan
Single binary
elreal_constants(build_dir/elastic/elreal/) covering:Leading-double round-trip vs
M_PI/M_E/M_LN2/M_LN10/M_SQRT24-component sum vs hard-coded 110-digit long-double reference at
1e-30relative tolerancecomputed_depth() == 4;at(k >= 4)is the implicit-zero extensionelreal_pi() > elreal(M_PI)at default budget -- Phase D's lazy-real distinctness applied to the constants: the correction bits inelreal_pi()make it observably greater than the IEEE roundedM_PI. Same for e (positive correction) and ln10 (negative correction).elreal_pi() != elreal(M_PI)by the same mechanism.gcc 13: 13 binaries (this new test + 12 prior) all PASS
clang: same
What does not land in this PR
numeric_limits<elreal>::pi()etc. accessor methods. The free-function naming (elreal_pi()) matches the project pattern (ereal_pi,dd_pi,qd_pi). A numeric_limits wrapper can be added later if generic code needs it.Related
Summary by CodeRabbit
New Features
Tests