NUMBERS-214: Bound the default max iterations for GeneralizedContinuedFraction - #219
Open
akashchamp wants to merge 1 commit into
Open
akashchamp wants to merge 1 commit into
akashchamp wants to merge 1 commit into
Conversation
…dFraction "GeneralizedContinuedFraction" used Integer.MAX_VALUE as the default number of iterations for the overloads of "value" (and the derived "ContinuedFraction.evaluate(double,double)") that do not accept an explicit maxIterations argument. For a fraction that does not converge this caused an ArithmeticException to be raised only after an excessive runtime (~12-15s for a simple non-converging generator on this hardware) instead of failing fast. Lower the default to 1,000,000, as suggested in the issue. A caller that requires more terms to converge can still use the existing 4-argument overloads with an explicit maxIterations; those overloads, and all current internal callers (BoostBeta, BoostGamma), already pass their own maxIterations and are unaffected by this change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
|
Thanks for the PR. I had prepared a patch already but was waiting for discussion on the Jira ticket. No discussion was forthcoming so I have merged my patch. |
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
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.
GeneralizedContinuedFractionusesInteger.MAX_VALUEas the defaultmaxIterationsfor thevalue(...)overloads that don't take an explicit iteration limit (andContinuedFraction.evaluate(double,double), which delegates to the same default). For a fraction that never converges, this means theArithmeticExceptionis only raised after iterating up to ~2^31 times, which can take many seconds instead of failing fast.This lowers the default to 1,000,000, as suggested in the issue.
Verification
Reproduced with the oscillating, non-converging generator from the issue (
b0 = 1, thena = 1, b = 0on every subsequent call):DEFAULT_ITERATIONS = 2147483647).DEFAULT_ITERATIONS = 1000000).Added a regression test with this generator that asserts the default-iteration overload throws without exceeding the documented default iteration count, plus a test that the default stays well below
Integer.MAX_VALUE.Ran
mvn testforcommons-numbers-fraction(all 158 tests pass, including the 2 new ones) and forcommons-numbers-gamma(all tests pass unaffected, since its call sites already pass an explicitmaxIterationsand are not affected by the default).The 4-argument overloads that accept an explicit
maxIterationsare unchanged, so callers who need more than 1,000,000 terms to converge can still opt in explicitly.