feat(elreal): Phase E.2 -- sqrt + hypot with depth-1 EFT refinement on sqrt#894
Merged
Conversation
…n sqrt Phase E.2 of epic #873 (resolves #888). The second math sub-issue: square root via Newton-style EFT residual at depth 1, plus a depth-0 hypot via std::hypot for overflow safety. sqrt(const elreal&): - Depth 0: c0 = std::sqrt(a.at(0)) (IEEE-754 correctly rounded) - Depth 1: Newton residual c1 ~= (a - c0^2) / (2*c0). a - c0^2 is computed with two_prod (giving c0^2 = prod_hi + prod_err exactly), then a single double subtraction picks up the operand's depth-1 correction a.at(1). The division by 2*c0 is plain double arithmetic on the elreal-internal numerator, NOT a lazy elreal divide -- so we are not blocked by Phase F's reciprocal-stream work. - Depth 2+: 0.0 (documented limitation; full Heron iteration on the lazy stream lands in Phase F). - Negative argument: throws elreal_negative_sqrt_arg when ELREAL_THROW_ARITHMETIC_EXCEPTION is set; otherwise returns NaN via std::sqrt(negative double). hypot(const elreal&, const elreal&): - Depth 0: c0 = std::hypot(a.at(0), b.at(0)) -- handles overflow, signed zero, infinities, and NaN per IEEE-754 conventions. The overflow-protected algebraic form (max(|a|,|b|) * sqrt(1 + (min/max)^2)) is built into the standard library implementation. - Depth 1+: deferred. Either an overflow-protected algebraic form walked at depth 1, or sqrt(a*a + b*b) when neither operand is near overflow -- a small follow-up. Tests under elastic/elreal/math/: sqrt.cpp -- perfect squares, non-square cross-validation vs std::sqrt, round-trip sqrt(a)^2 ~= a, sqrt(2) matches std::numbers::sqrt2_v, depth-1 propagation on rational input, negative -> NaN, special values sqrt_throw.cpp -- elreal_negative_sqrt_arg in the throw mode (separate binary because the macro is compile-time fixed) hypot.cpp -- Pythagorean triples, zero/symmetry/negative args, overflow safety at 1e200, cross-validation vs std::hypot, special-value propagation 16 elreal binaries (3 new + 13 prior) all PASS on gcc 13 and clang. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
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 (2)
📝 WalkthroughWalkthroughThis PR adds Phase E.2 math functions to the ChangesPhase E.2 Math Functions: sqrt and hypot for elreal
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Suggested labelsenhancement 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
elastic/elreal/math/sqrt.cpp (1)
86-121: ⚡ Quick winAdd regression coverage for leading-zero expansion inputs.
Please add cases like
elreal::from_expansion({0.0, +eps})andelreal::from_expansion({0.0, -eps})so the suite validates positive tiny values and negative detection when depth-0 is zero.🤖 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/sqrt.cpp` around lines 86 - 121, Add two regression tests near the existing sqrt cases: create elreal numbers using elreal::from_expansion({0.0, +eps}) and elreal::from_expansion({0.0, -eps}) (with eps = std::numeric_limits<double>::epsilon()), then call sqrt(...) on each. For the positive tiny case assert the sqrt result is finite/non-NaN (and optionally that r.at(0) >= 0 and r.at(1) != 0 or that (r*r) is close to eps via check_close). For the negative tiny case assert sqrt returns NaN (use r.isnan()) like the existing negative-argument test. Use the same style/scheme as the surrounding tests and reference elreal::from_expansion, sqrt, at(), isnan(), and check_close to locate where to add these checks.
🤖 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 `@include/sw/universal/number/elreal/elreal_impl.hpp`:
- Around line 922-937: The sqrt implementation currently only examines the
depth-0 limb (a0 / a.at(0)) which produces incorrect behavior for expansions
whose leading limb is zero (e.g. {0.0, c1, ...}); change the logic in the sqrt
path to locate the first non-zero, finite materialized limb (or call the
existing normalization/materialization routine) instead of using a.at(0)
directly, then base the negative-argument check and the initial c0 =
std::sqrt(...) on that first non-zero limb; ensure you update the resulting
elreal _components and _computed_depth the same way after using that normalized
leading limb (keep elreal, _components, _computed_depth, and the
negative-argument throw/NaN handling intact).
---
Nitpick comments:
In `@elastic/elreal/math/sqrt.cpp`:
- Around line 86-121: Add two regression tests near the existing sqrt cases:
create elreal numbers using elreal::from_expansion({0.0, +eps}) and
elreal::from_expansion({0.0, -eps}) (with eps =
std::numeric_limits<double>::epsilon()), then call sqrt(...) on each. For the
positive tiny case assert the sqrt result is finite/non-NaN (and optionally that
r.at(0) >= 0 and r.at(1) != 0 or that (r*r) is close to eps via check_close).
For the negative tiny case assert sqrt returns NaN (use r.isnan()) like the
existing negative-argument test. Use the same style/scheme as the surrounding
tests and reference elreal::from_expansion, sqrt, at(), isnan(), and check_close
to locate where to add these checks.
🪄 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: 74761c71-3c25-4454-ad6a-bb075477370b
📒 Files selected for processing (4)
elastic/elreal/math/hypot.cppelastic/elreal/math/sqrt.cppelastic/elreal/math/sqrt_throw.cppinclude/sw/universal/number/elreal/elreal_impl.hpp
Address CodeRabbit's Major finding on PR #894 (real bug). sqrt was checking only a.at(0) for both the negative-argument guard and the leading sqrt computation. For expansions whose leading double cancels in arithmetic -- {0.0, c1, ...} where c1 != 0 -- this misclassifies: - {0, +eps}: a small positive value; pre-fix sqrt returned 0 because std::sqrt(0.0) == 0, hiding the true sqrt(eps) ~= 3.16e-5. - {0, -eps}: a small negative value; pre-fix sqrt returned 0 because a.at(0) == 0 bypassed the negative-argument handler, suppressing the expected NaN / elreal_negative_sqrt_arg. The canonical input that triggers the bug is the lazy-real distinctness test from Phase D: `elreal(1, 3) - elreal(1.0/3.0)` cancels at depth 0 and leaves a positive depth-1 correction equal to the rational residual. sqrt of that should be ~sqrt(1.85e-17) = ~4.3e-9, not zero. Fix: walk the materialised components to find the first non-zero one. That component drives both the sign check and the leading sqrt. The depth-1 generator captures the index so the EFT residual formula uses the right "leading" and the right next-correction limb. For the common case (lead_idx == 0) this collapses to the original formula. Three new regression tests in elastic/elreal/math/sqrt.cpp cover the fix: - sqrt(elreal(1, 3) - elreal(1.0/3.0)) round-trips to recover the input - sqrt({0, -1e-9}) produces NaN - sqrt({0, +1e-9}) has leading ~= 3.16e-5 Each of the three would have failed against the pre-fix code. All 16 elreal binaries PASS on gcc 13 and clang. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
github-project-automation
Bot
moved this from In progress
to Done
in Universal Number Library
Coverage Report for CI Build 26223836575Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage increased (+0.006%) to 84.233%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions8 previously-covered lines in 1 file lost coverage.
Coverage Stats
💛 - Coveralls |
9 tasks
This was referenced May 22, 2026
This was referenced Jun 2, 2026
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
2 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
Phase E.2 of epic #873. Implements #888 (the second math sub-issue under Phase E #878).
Square root via Newton-style EFT residual at depth 1, plus a depth-0 hypot via
std::hypotfor overflow safety. No dependency on Phase F's lazy division -- the sqrt depth-1 correction divides by2*c0(a single double), not by a lazy elreal, so it works under the Phase C / Phase F constraint that lazy/is depth-0 only.sqrtalgorithmNewton's identity:
(c0 + delta)^2 = a => delta = (a - c0^2) / (2*c0 + delta) ~= (a - c0^2) / (2*c0)for|delta| << c0.So depth-1 captures both the EFT residual of
c0^2and the operand's own depth-1 correctiona.at(1). Depth 2+ returns 0.0 (Heron iteration on the lazy stream is a Phase F follow-up).Negative argument: throws
elreal_negative_sqrt_argunderELREAL_THROW_ARITHMETIC_EXCEPTION, else returns NaN viastd::sqrton the leading double.hypotalgorithmstd::hypot(a.at(0), b.at(0))at depth 0. The standard library implementation handles overflow (large operands), signed zero, infinities, and NaN per IEEE-754 -- no need to re-derive themax * sqrt(1 + (min/max)^2)form here. Depth-1+ deferred; could be wired up later viasqrt(a*a + b*b)(when neither operand is near overflow) or the algebraic overflow-protected form walked at depth 1.Test plan
Three new binaries under
elastic/elreal/math/:elreal_sqrtstd::sqrt, round-tripsqrt(a)^2 ~= a,sqrt(2)matchesstd::numbers::sqrt2_v, depth-1 propagation on rational input (sqrt(2/3)), negative -> NaN, special valueselreal_sqrt_throwelreal_negative_sqrt_argraised whenELREAL_THROW_ARITHMETIC_EXCEPTIONis set; separate binary because the macro is fixed at compile timeelreal_hypotstd::hypot, special-value propagationWhat does not land in this PR
sqrt(Phase F)hypot(small follow-up; the algebraic form needs care to stay overflow-safe at depth 1)Related
Summary by CodeRabbit
New Features
Tests