Scan the bracket interior so even-crossing series find a root - #24
Merged
Conversation
Bracketing compared only the two ends of the rate domain, so a series with more than one IRR — where both ends share a sign — reported did_not_converge even though a real rate existed (numpy-financial #39). The bracket now scans the interior on a geometric grid and returns the sign-change interval nearest :guess, so a multi-IRR series resolves to the rate a guess-driven spreadsheet XIRR returns. Both solvers share the bracket. Single-rate series are unchanged. Ships under 1.6.0 alongside Finance.Solver.Brent. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AVibXT2w1dEdVtMHqffFKn
|
coverage: 100.0%. remained the same — bracket-interior-scan into main |
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.
The bug
Finance.Shared.bracket/1compared only the two ends of the rate domain: it fixed the NPV at the floor and doubledhigh, checking those two endpoints for a sign change. When the NPV crosses zero an even number of times — a series with more than one IRR, where both ends share a sign — no bracket was ever found and the solve returned{:error, :did_not_converge}, even though a real, economically meaningful rate exists.Reproduced against the numpy-financial #39 series (IRRs near −1.8% and 12%, NPV negative at both ends of the domain):
Finance.CashFlow.irr/1returned:did_not_convergeonmainwhere Excel/Sheets compute 12%.The fix
Bracketing now scans the interior of the domain on a geometric grid (5% steps in
1 + rate) and, among every adjacent-sample sign change, returns the interval whose nearer edge is closest to:guess. So::did_not_converge).XIRRwould return, not always the lowest. The default guess (0.1) reproduces[-50,-100,600,300,-100] → -76.9%(numpy Guard against rates at or below -100% and fix rounding in ytm #28) and[-1600,10000,-10000] → 0.25;guess: 3.0on the latter selects the far root4.0.Finance.Solver.NewtonandFinance.Solver.Brentshare the bracket, so they select the same root (parity-tested).did_not_convergecases (net-positive / no sign change) still correctly diverge.The floor logic, overflow/underflow safety, and expansion cap are preserved — only sign-change detection changed, not the domain math.
Tests
New regression coverage: the numpy #39 even-crossing case, guess-anchored root selection (both solvers), and real-world cash flows that broke other implementations (nodejs-xirr #2, the Ruby finance gem's 56y/23y horizons, peliot's negative-base crash class), plus the zero-payout / same-date / leap-year day-count conventions.
233 tests across seeds,
credo --strictand dialyzer clean, 100% coverage, 0 doc warnings. Ships under 1.6.0 alongsideFinance.Solver.Brent.🤖 Generated with Claude Code