Re: [PR] IRR: fall back to bracketed Newton-Raphson when t he iteration diverges (bug 64137 follow-up) [poi]

pjfanning commented on PR #1216:
URL: https://github.com/apache/poi/pull/1216#issuecomment-5305209028

   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
   
   1. Bug: noValidIrrReturnsNaN test for {-1, 3, -2.5} is incorrect — TestIrr.java
   
   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(0) = -1 + 3 - 2.5 = -0.5
   - NPV(0.1) = -1 + 3/1.1 - 2.5/1.21 ≈ -1 + 2.727 - 2.066 ≈ -0.339
   - NPV(0.5) = -1 + 3/1.5 - 2.5/2.25 ≈ -1 + 2 - 1.111 ≈ -0.111
   - NPV(1.0) = -1 + 3/2 - 2.5/4 = -1 + 1.5 - 0.625 = -0.125
   - NPV(5.0) = -1 + 3/6 - 2.5/36 ≈ -1 + 0.5 - 0.069 ≈ -0.569
   
   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
   
   1. Minor: Grid gap between 100 and 10000 — Irr.java:60
   
   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.
   
   2. Nit: flo variable is unused after the bracket orientation swap — Irr.java:190-195
   
   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.
   
   3. Style: newtonIrr method is private but bracketedIrr calls npv/npvDerivative — Irr.java
   
   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.
   
   4. Question: Why not also guard newtonIrr against rates ≤ -1? — Irr.java:121-124
   
   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
   
   - The rtsafe algorithm is textbook-correct. Newton steps when they stay in the bracket and converge fast enough, bisection otherwise. The convergence check Math.abs(2.0 * f) > Math.abs(dxold * df) correctly ensures each step at least halves the bracket.
   - The npv() method using y = 1/(1+x) is a smart choice — near -1 it overflows to ±∞ (still usable for sign tests) rather than underflowing a shared denominator to 0.
   - The findBracket grid near -1 (points at -1 + 10^-9 through -1 + 0.01) is well-chosen for catching roots pushed toward -100% by extreme cash flows.
   - Backward compatibility is preserved. newtonIrr is byte-for-byte the old algorithm; existing results are unchanged.
   - Test coverage for the two failure modes (divergence, meaningless rate) and the no-valid-IRR case is good.
   - The logging relocation makes sense — the old "Returning NaN" logs are no longer accurate at the Newton stage since NaN is now just an internal signal.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]