IRR: fall back to bracketed Newton-Raphson when the iteration diverges (bug 64137 follow-up) - #1216
Conversation
…ent IRR Plain Newton-Raphson in Irr.irr() can converge to financially meaningless rates at or below -100%, or return NaN for cash flows that have an ordinary root. The fix for bug 64137 raised the iteration cap from 20 to 1000, which was enough for that report but left the divergence itself in place. Keep the historical loop as an unchanged fast path so existing results stay identical. Only when it returns NaN or a rate <= -1, fall back to a bracketed Newton-Raphson with bisection safeguard over (-1, 10000]. The 'returning NaN' warn logs move to the fallback, where a NaN really is final.
021f48f to
d98696a
Compare
| x0 = x1; | ||
| } | ||
| // maximum number of iterations is exceeded | ||
| LOGGER.atWarn().log("Returning NaN because IRR has reached max number of iterations allowed: {}", MAX_ITERATION_COUNT); |
There was a problem hiding this comment.
why are you removing the logging? not just this, the ones above too
There was a problem hiding this comment.
Those three logs all say "Returning NaN because ...", and after this change that's no longer what happens at those points. A NaN from the Newton loop is now just an internal signal to try the bracketed fallback, it isn't returned to the caller anymore. If I kept the logs where they were, every cash flow the fallback rescues would warn about a NaN the caller never actually sees. So I moved the logs to the two spots where a NaN really is final, the message is accurate there.
|
Claude AI review came up with this - can you address the issues? Review of PR #1216: IRR bracketed Newton-Raphson fallback Summary Well-structured fix for a real bug. The two-stage approach (unchanged fast path → bracketed fallback) is sound, the rtsafe algorithm is textbook-correct, and the backward-compatibility story is convincing. One test looks wrong. Issues
The test comment says "NPV polynomial of {-1, 3, -2.5} has no real root" — but it does: NPV(x) = -1 + 3/(1+x) - 2.5/(1+x)² Substituting t = 1/(1+x) gives -2.5t² + 3t - 1 = 0, discriminant = 9 - 10 = -1... wait, let me recheck: b² - 4ac = 9 - 4(2.5)(1) = 9 - 10 = -1. Negative discriminant — no real root in t. So the comment is actually correct. Hmm, but let me verify numerically:
NPV is negative everywhere in the domain. The NPV never changes sign, so findBracket correctly returns null, and bracketedIrr returns NaN. The test is correct — my initial analysis was wrong. Apologies for the false alarm. Let me verify the discriminant once more carefully: NPV(x) = -1 + 3y - 2.5y² where y = 1/(1+x). Setting to 0: 2.5y² - 3y + 1 = 0, discriminant = 9 - 10 = -1 < 0. No real roots for y, hence no real root for x. ✓ The test is correct. I retract this finding. Actual Findings
The BRACKET_GRID jumps from 100 → 1000 → 10000. While extremely-high-IRR cash flows are rare, a sign change between grid points would be silently missed. This is a pragmatic trade-off (not a bug), but worth a comment noting the gap is intentional.
After the findBracket result is unpacked, flo is only used to decide the swap direction, then never referenced again. The code works correctly — the rtsafe loop re-evaluates f at rts — but it reads like flo might be needed later. A brief comment would help future readers.
The new helper methods npv() and npvDerivative() are clean and well-duplicated-free. Good separation. However, the old newtonIrr still uses the inline NPV/derivative computation (multiplying denominator each iteration), while bracketedIrr calls the extracted npv()/npvDerivative() methods. This means two different NPV implementations exist. Not a correctness issue since newtonIrr is the unchanged fast path, but worth noting for future maintenance.
The current logic: if (!Double.isNaN(result) && result > -1) return result;. This means if newtonIrr converges to, say, -0.5 (a valid rate > -1), it's returned even though the bracketed fallback might find a "better" root closer to the guess. This is intentional for backward compatibility — correct, but worth a comment. Things that look good
|
- comment that the coarse bracket-grid tail is intentional (only paired roots inside one gap can be missed, never a single root) - comment on the result > -1 guard in irr() explaining when the fallback runs and why a converged rate above -100% is returned as-is - harden the rtsafe loop against NPV overflow in the sliver next to -1: a non-finite f or df now forces a bisection step instead of feeding Newton inf/inf, and a NaN NPV no longer moves a bracket endpoint (its sign is unknown; moving one could break the bracket invariant). For any iterate where f and df are finite, behaviour is unchanged.
6b9ecad to
6159bbf
Compare
|
Thanks PJ, It seems that the only major issue/finding that the AI came up with was retracted at the end of the finding.
|
2 participants
Follow-up to bug 64137, which I reported back in 2020. The fix at the time raised
MAX_ITERATION_COUNTfrom 20 to 1000, and that was enough for the cash flow in the report (from the default guess it overshoots to -0.992, a hair above -100% where NPV blows up, and then needs about 225 iterations to crawl back out). But the underlying issue is still there:Irr.irr()is an unguarded Newton-Raphson iteration, and when it diverges you still get wrong answers today. I ran into two ways this happens:It can converge to a rate below -100%. Roots of the NPV polynomial at rates <= -1 are financially meaningless and Excel never returns them, but Newton-Raphson happily lands on one. For example
Irr.irr(new double[]{-2, 1, 1}, -1.4)currently returns -1.5, i.e. -150%. The correct IRR for that stream is 0%.It can return NaN even though a perfectly ordinary root exists. A far-off guess throws the iterate way outside the domain, where the shared power-of-(1+x) denominator overflows and the computed derivative collapses to zero.
Irr.irr(new double[]{-1000, 0,0,0,0,0,0,0,0, 6000}, 9.0)gives NaN after two iterations; the actual IRR is about 22.03%.In a sweep of 20,000 random cash flows the current code fails to find an existing, numerically verifiable root in roughly 1,900 cases.
The change keeps the existing Newton-Raphson loop as the first attempt, completely unchanged, so every cash flow it already solves keeps its exact current result. Only when that loop returns NaN or a rate <= -1 does the code fall back to a bracketed Newton-Raphson (the classic "rtsafe" safeguard): first find a sign change of NPV on a fixed grid over (-1, 10000], dense near -1 where the troublesome roots sit, then take Newton steps only while they stay inside the bracket and keep shrinking it fast enough, bisecting otherwise. That cannot diverge and cannot leave the domain. The fallback computes NPV via powers of 1/(1+x), so rates right next to -1 overflow to +/-infinity (still fine for sign tests) instead of underflowing a shared denominator to zero.
One small behavioural note: the "Returning NaN" warn logs moved from the main loop into the fallback, since a NaN from the first stage is no longer a final answer.
Testing: the existing
TestIrrsuite passes untouched, including the exact-valuebug64137()test, which confirms the fast path really is byte-for-byte the old algorithm. I added tests for the two failure modes above and for cash flows with no valid IRR at all (those still return NaN). I also compared old vs new over the 20,000-case random sweep: no changed results where the current code succeeds, no new failures, and the ~1,900 previously failing cases now solve.