Improve expo analytical integral stability

[swetank18]

Motivation

The analytical integral of exponential TF1 functions computes differences of
exponentials, which can suffer from numerical cancellation or overflow when the
integration bounds are close or the exponent is large.

Changes

• Replace the direct difference of exponentials with a numerically stable
  expm1-based formulation

Impact

• Improves numerical robustness without changing the mathematical result
• No API changes

[hageboeck]

This will clash with #20992. Should we close that one?

[swetank18]

This will clash with #20992. Should we close that one?

Thanks for pointing this out.
Both PRs are mine — the earlier one (#20992) introduced the initial change, and this PR refines it with improved numerical stability.
If you prefer, I’m happy to close #20992 and keep everything here, or alternatively rebase/split to avoid overlap — whatever you think is cleaner.

[hageboeck]

If you prefer, I’m happy to close #20992 and keep everything here, or alternatively rebase/split to avoid overlap — whatever you think is cleaner.

Let's move it here, so we can save a few CI runs.

[github-actions]

Test Results

    22 files      22 suites   3d 7h 39m 51s ⏱️
 3 794 tests  3 794 ✅ 0 💤 0 ❌
76 312 runs  76 312 ✅ 0 💤 0 ❌

Results for commit f9d6908.

♻️ This comment has been updated with latest results.

[hageboeck]

The tests seem to fail because the commit from #20993 is not in here, but the tests rely on this behaviour. So maybe we should do the following:

• Cherry-pick and rebase.
  • Interactive rebase, squashing the "stabilise" commit with its clang-format.
  • Cherry-pick the commit from Add parameter domain checks to analytical integrals #20993 into this branch.
  • And put the tests last, so everything stays green in between these commits.
  • And maybe update the PR description to reflect these changes.
• Alternatively, one could move the parameter domain test into Add parameter domain checks to analytical integrals #20993, but I doubt that this is less work.

@swetank18, what do you think?

[swetank18]

The tests seem to fail because the commit from #20993 is not in here, but the tests rely on this behaviour. So maybe we should do the following:

* Cherry-pick and rebase.
  
  * Interactive rebase, squashing the "stabilise" commit with its clang-format.
  * Cherry-pick the commit from [Add parameter domain checks to analytical integrals #20993](https://github.com/root-project/root/pull/20993) into this branch.
  * And put the tests last, so everything stays green in between these commits.
  * And maybe update the PR description to reflect these changes.

* Alternatively, one could move the parameter domain test into [Add parameter domain checks to analytical integrals #20993](https://github.com/root-project/root/pull/20993), but I doubt that this is less work.

@swetank18, what do you think?

Thanks, that makes sense to me.
I’m happy to follow that approach: I’ll consolidate the commits with an interactive rebase, cherry-pick the parameter domain checks from #20993, and ensure the tests come last so CI stays green throughout. I’ll also update the PR description accordingly.

[swetank18]

The tests seem to fail because the commit from #20993 is not in here, but the tests rely on this behaviour. So maybe we should do the following:

* Cherry-pick and rebase.
  
  * Interactive rebase, squashing the "stabilise" commit with its clang-format.
  * Cherry-pick the commit from [Add parameter domain checks to analytical integrals #20993](https://github.com/root-project/root/pull/20993) into this branch.
  * And put the tests last, so everything stays green in between these commits.
  * And maybe update the PR description to reflect these changes.

* Alternatively, one could move the parameter domain test into [Add parameter domain checks to analytical integrals #20993](https://github.com/root-project/root/pull/20993), but I doubt that this is less work.

@swetank18, what do you think?
Apologies for the delay — I was unwell for a few days.
I’ve now completed the consolidation as discussed and updated the branch accordingly. Please let me know if you’d like anything adjusted.

[hageboeck]

Apologies for the delay — I was unwell for a few days.
I’ve now completed the consolidation as discussed and updated the branch accordingly. Please let me know if you’d like anything adjusted.

Hello, no problem!

The total code changes look good, but could you squash the last three commits (the tests) into one? Alternatively, we can squash the entire work into one commit when we merge, but I think it's reasonable to have the code changes and the tests in separate commits.

[hageboeck]

The total code changes look good, but could you squash the last three commits (the tests) into one?

And in doing that, could you run git clang-format on the changes, please? 🙂
The formatting check failed because of this.

[swetank18]

The total code changes look good, but could you squash the last three commits (the tests) into one?

And in doing that, could you run git clang-format on the changes, please? 🙂 The formatting check failed because of this.

I’ve squashed the test commits into a single commit, run git clang-format on the branch, and force-pushed the updated history. Please let me know if anything else should be adjusted.

[hageboeck]

The total code changes look good, but could you squash the last three commits (the tests) into one?

And in doing that, could you run git clang-format on the changes, please? 🙂 The formatting check failed because of this.

I’ve squashed the test commits into a single commit, run git clang-format on the branch, and force-pushed the updated history. Please let me know if anything else should be adjusted.

The test you implemented uses a numeric integrator from the gsl and this fails:

[ RUN      ] TF1AnalyticalIntegral.GaussianInvalidSigma
  /github/home/ROOT-CI/src/core/testsupport/src/TestSupport.cxx:93: Failure
  Failed
  Received unexpected diagnostic of severity 3000 at 'GSLError' reading 'Error 11 in qags.c at 543 : number of iterations was insufficient'.
  Suppress those using ROOT/TestSupport.hxx
  /github/home/ROOT-CI/src/core/testsupport/src/TestSupport.cxx:93: Failure
  Failed
  Received unexpected diagnostic of severity 2000 at 'TF1::IntegralOneDim' reading 'Error found in integrating function f_gaus_invalid in [-1.000000,1.000000] using AdaptiveSingular. Result = 0.000000 +/- nan  - status = 11'.
  Suppress those using ROOT/TestSupport.hxx
  /github/home/ROOT-CI/src/hist/hist/test/test_tf1.cxx:115: Failure
  Value of: std::isnan(result)
    Actual: false
  Expected: true
  [  FAILED  ] TF1AnalyticalIntegral.GaussianInvalidSigma (90 ms)

It seems that the analytic integral didn't get used.

[swetank18]

The total code changes look good, but could you squash the last three commits (the tests) into one?

And in doing that, could you run git clang-format on the changes, please? 🙂 The formatting check failed because of this.

I’ve squashed the test commits into a single commit, run git clang-format on the branch, and force-pushed the updated history. Please let me know if anything else should be adjusted.

The test you implemented uses a numeric integrator from the gsl and this fails:

[ RUN      ] TF1AnalyticalIntegral.GaussianInvalidSigma
  /github/home/ROOT-CI/src/core/testsupport/src/TestSupport.cxx:93: Failure
  Failed
  Received unexpected diagnostic of severity 3000 at 'GSLError' reading 'Error 11 in qags.c at 543 : number of iterations was insufficient'.
  Suppress those using ROOT/TestSupport.hxx
  /github/home/ROOT-CI/src/core/testsupport/src/TestSupport.cxx:93: Failure
  Failed
  Received unexpected diagnostic of severity 2000 at 'TF1::IntegralOneDim' reading 'Error found in integrating function f_gaus_invalid in [-1.000000,1.000000] using AdaptiveSingular. Result = 0.000000 +/- nan  - status = 11'.
  Suppress those using ROOT/TestSupport.hxx
  /github/home/ROOT-CI/src/hist/hist/test/test_tf1.cxx:115: Failure
  Value of: std::isnan(result)
    Actual: false
  Expected: true
  [  FAILED  ] TF1AnalyticalIntegral.GaussianInvalidSigma (90 ms)

It seems that the analytic integral didn't get used.

Thanks for the detailed output. You're right — the test triggered the numeric GSL integrator instead of the analytic path. Since this behavior isn’t guaranteed to return NaN, I’ve removed the invalid-sigma test to avoid relying on fallback behavior.

[hageboeck]

Thanks for the detailed output. You're right — the test triggered the numeric GSL integrator instead of the analytic path. Since this behavior isn’t guaranteed to return NaN, I’ve removed the invalid-sigma test to avoid relying on fallback behavior.

For future reference, the reason why returning NaN from the analytic integral function doesn't work:
The analytic integral helpers are implemented such that when the "analytic attempt" fails with NaN, the numeric integrator is invoked.
An exponential with p[1] == 0 never was a problem. You got a result, but it was a numeric integration. Now, we directly get the analytic result, so thanks for this contribution!

For the Gaussian, for the same reasons, the numeric integrator is invoked, so the test wasn't supposed to work.