Skip to content

Conversation

@swetank18
Copy link

@swetank18 swetank18 commented Jan 28, 2026

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

@swetank18 swetank18 requested a review from hageboeck as a code owner January 28, 2026 18:24
@hageboeck
Copy link
Member

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

@hageboeck hageboeck self-assigned this Jan 30, 2026
@swetank18
Copy link
Author

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
Copy link
Member

hageboeck commented Feb 2, 2026

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

github-actions bot commented Feb 2, 2026

Test Results

    22 files      22 suites   3d 9h 15m 22s ⏱️
 3 788 tests  3 786 ✅ 0 💤  2 ❌
75 270 runs  75 249 ✅ 0 💤 21 ❌

For more details on these failures, see this check.

Results for commit dcf9bb8.

♻️ This comment has been updated with latest results.

@hageboeck
Copy link
Member

hageboeck commented Feb 2, 2026

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:

@swetank18, what do you think?

@swetank18
Copy link
Author

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 swetank18 force-pushed the improve-expo-analytical-integral-stability branch from f3dcfcc to dcf9bb8 Compare February 7, 2026 18:49
@swetank18
Copy link
Author

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
Copy link
Member

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
Copy link
Member

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.

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