Skip to content

NUMBERS-214: Bound the default max iterations for GeneralizedContinuedFraction - #219

Open
akashchamp wants to merge 1 commit into
apache:masterfrom
akashchamp:NUMBERS-214-bound-default-iterations
Open

akashchamp wants to merge 1 commit into
apache:masterfrom
akashchamp:NUMBERS-214-bound-default-iterations

Conversation

@akashchamp

Copy link
Copy Markdown

GeneralizedContinuedFraction uses Integer.MAX_VALUE as the default maxIterations for the value(...) overloads that don't take an explicit iteration limit (and ContinuedFraction.evaluate(double,double), which delegates to the same default). For a fraction that never converges, this means the ArithmeticException is 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, then a = 1, b = 0 on every subsequent call):

  • Before this change: throws after ~14.8s on this machine (DEFAULT_ITERATIONS = 2147483647).
  • After this change: throws after well under a second (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 test for commons-numbers-fraction (all 158 tests pass, including the 2 new ones) and for commons-numbers-gamma (all tests pass unaffected, since its call sites already pass an explicit maxIterations and are not affected by the default).

The 4-argument overloads that accept an explicit maxIterations are unchanged, so callers who need more than 1,000,000 terms to converge can still opt in explicitly.

…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>
@aherbert

Copy link
Copy Markdown
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants