[Math] Laplacian: error in the finite differences formulas

[dpiparo]

Fix a few typos and a mistake in Laplacian calculation.
The standard second-order central, backward and forward differences contained a mistake: a factor-of-4 error.

See finite different coefficients here: https://en.wikipedia.org/wiki/Finite_difference_coefficient

[github-actions]

Test Results

    23 files      23 suites   3d 15h 10m 44s ⏱️
 3 873 tests  3 873 ✅ 0 💤 0 ❌
78 786 runs  78 786 ✅ 0 💤 0 ❌

Results for commit 03b8ecd.

[guitargeek]

Thanks for the fixes! I'm unsure if they should go in like this though.

I think we should either:

1. Also write a test
2. Remove TMath::Laplacian. If a function gave silently wrong results and nobody complained, it was apparently not important. This would also mitigate the risk that people get confused by getting silently different results from the same code (that alone also means that this change would have to be mentioned in the release notes).

Personally, I would just delete the code if it really gave wrong results, along with its sibling function TMath::Gradient for consistency. But I'd also be fine with keeping it, as long as a test is implemented and release notes mention the silent math change.

[guitargeek]

Sorry, I looked into the history only now and realized this function was only introduced 2 years ago, and requested by @ferdymercury:

• [TMath] Add Gradient and Laplacian methods for arrays #15100

In that case, I'd give it more of a chance to stay in ROOT as long as we have a test.

[ferdymercury]

as long as we have a test.

It seems the test is already there? math/mathcore/test/testTMath.cxx

But maybe it's not running correctly?

[hageboeck]

as long as we have a test.

It seems the test is already there? math/mathcore/test/testTMath.cxx

But maybe it's not running correctly?

I didn't find the test (not searching correctly), so I had already written a new one. Clearly, the Laplace function was wrong:

The green line should be identical to the marker.

Now that the test was written, I guess that I could as well just add it.

[dpiparo]

I think the test of TMath::Laplacian just checks for identity against a off-by-four reference... Those numbers seem to come from ROOT's implementation and not a reference one.

[ferdymercury]

I think the test of TMath::Laplacian just checks for identity against a off-by-four reference... Those numbers seem to come from ROOT's implementation and not a reference one.

But shouldn't then the test have started failing after 03b8ecd ?

[hageboeck]

I think the test of TMath::Laplacian just checks for identity against a off-by-four reference... Those numbers seem to come from ROOT's implementation and not a reference one.

But shouldn't then the test have started failing after 03b8ecd ?

The test fails, but it doesn't signal any failure to the outside. It just prints errors and then exit(0). I'll improve that tomorrow.

[hageboeck]

@dpiparo pushing the test didn't work as hoped, so maybe you'd better remove that commit.

An updated version of the test, as well as an improvement of the doxygen is here now:
#22823

We could stagger the merges, or you can cherry-pick. Whatever is more convenient.