Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Nan/infinite guard to gauss_kronrod_quadrature (early exit) #59

Merged
merged 2 commits into from
Apr 25, 2024

Conversation

GComitini
Copy link
Contributor

Hello there! While I haven't had time yet to think about the complex Lagrange polynomials, I would like to propose a "performance improvement" for gauss_kronrod_quadrature.

Today my program got stuck while using the latter to do numerical integrations. After some digging I found out that G and K in gauss_kronrod_quadrature had been equal to NaN for a very large number of iterations and/or interval subdivisions. Ultimately, the reason why my program got stuck was issues of convergence in the integral itself. However, if gauss_kronrod_quadrature had detected those issues (to be specific: those NaNs) earlier, the program would have failed immediately instead of having to exhaust all iterations for all the interval subdivisions.

Looking at the code,

if (G - K).abs() < tol_curr || a == b || max_iter == 0 {
    I += G;
} else {
    S.push((a, c, tol / 2f64, max_iter - 1));
    S.push((c, b, tol / 2f64, max_iter - 1));
}

if G is infinite or Nan (in which case, for the record, (G - K).abs() < tol_curr is false), and if we either reached the end of precision in the interval's subdivision (a == b), or we exhausted the maximum number of iterations for the given subinterval (max_iter == 0), then G will be added to I and the loop will go on. At this point, however, I, which is the final result, is already tainted: it will be either infinite or NaN and further operating on it during later iterations of the loop won't change this. Thus, if ! G.is_finite() (that is, if G is either infinite or Nan) before adding G to I, one could as well exit early with whatever meaningless result one has at that point. If I is finite and G is not finite, I + G = G, so one may as well return G; at that point.

To be very clear, this change constitutes a performance improvement only for integrals whose computation fails, by making some of them fail faster. What needs to be evaluated is the performance impact on well-behaved integrals. If you have benchmarking code in place I will be happy to run it on this MR branch and see what happens.

Axect and others added 2 commits April 16, 2024 14:29
- Do not include legend box if there is no legend (Axect#58)
- Add rtol for Broyden method
- High-level macros for root finding
@Axect
Copy link
Owner

Axect commented Apr 25, 2024

Hi @GComitini,

Thank you for your proposal to improve the performance of gauss_kronrod_quadrature in case of convergence issues. Your suggestion to detect NaN values early and exit the loop to avoid unnecessary iterations is a good one.

I agree that adding a check like if !G.is_finite() before updating I could help fail faster for problematic integrals without impacting the results. It's a reasonable optimization to consider.

Regarding the performance impact on well-behaved integrals, I don't expect it to be significant since the additional check is relatively cheap.

I'll merge it now and release next version soon including this change.

Thanks again for taking the time to analyze the issue and propose a solution!

@Axect Axect changed the base branch from master to dev April 25, 2024 04:13
@Axect Axect merged commit 6804677 into Axect:dev Apr 25, 2024
Axect added a commit that referenced this pull request May 1, 2024
- Add Nan/infinite guard to `gauss_kronrod_quadrature` (early exit) (#59)
- Add complex feature & complex module (#35)
- Implement Cubic B-Spline basis function
Axect added a commit that referenced this pull request May 1, 2024
- Add Nan/infinite guard to `gauss_kronrod_quadrature` (early exit) (#59)
- Add complex feature & complex module (#35)
- Implement Cubic B-Spline basis function
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.

None yet

2 participants