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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
