Mutable polynomial roots

[meiyasan]

This Pull request:

Hi @lmoneta @vepadulano ; we may benefit from introducing a mutable keyword to fRoots in the Polynomial.h methods. I would be grateful if you could consider such changes as described below.

Changes or fixes:

I applied a mutable keywords to fRoots allowing to use FindRoots in const context. Such keyword was already applied along the same line to fDerived_params variable in math/mathmore/inc/Math/Polynomial.h.

Checklist:

• tested changes locally
• updated the docs (if necessary)

Tests:

I ran the ROOT battery of tests with couple of issues (related to some stress test, vectorization & simultions), but those also happened on my Mac M3 ARM before my modifications. If you have any concern, I believe it wouldn't be an issue to run it in a more standard dev context.

cmake -DCMAKE_BUILD_TYPE=Debug -Dtesting=ON -Droottest=ON -Dmathmore=ON ../polynomial-patch
make -j8
ctest -j8

Regarding the Polynomial class modifications, my use case output remains unchanged. (I use polynomials to compute digital filtering, biquad, etc..) I also paid attention in particular to those related to polynomials: gtest-hist-hist-testTProfile2Poly, etc..

1015/3593 Test  #155: gtest-graf2d-primitivesv7-graf2dprimitivesv7testUnit ..............................................   Passed    0.39 sec
          Start  158: gtest-hist-hist-testTProfile2Poly
1016/3593 Test  #156: gtest-graf3d-eve7-graf3deve7testUnit ..............................................................   Passed    0.46 sec
          Start  159: gtest-hist-hist-testTFractionFitter
1017/3593 Test  #159: gtest-hist-hist-testTFractionFitter ...............................................................   Passed    0.39 sec
          Start  160: gtest-hist-hist-testTH2PolyBinError
1018/3593 Test  #160: gtest-hist-hist-testTH2PolyBinError ...............................................................   Passed    0.30 sec
          Start  161: gtest-hist-hist-testTH2PolyAdd
1019/3593 Test   #84: pyunittests-bindings-pyroot-pythonizations-pyroot-pyz-rdataframe-histo-profile ....................   Passed   13.81 sec
          Start  162: gtest-hist-hist-testTH2PolyGetNumberOfBins
1020/3593 Test  #162: gtest-hist-hist-testTH2PolyGetNumberOfBins ........................................................   Passed    0.24 sec
          Start  163: gtest-hist-hist-testTHn
1021/3593 Test  #161: gtest-hist-hist-testTH2PolyAdd ....................................................................   Passed    0.51 sec
[...]

1198/3593 Test  #337: gtest-roofit-roofit-vectorisedPDFs-testNestedPDFs .................................................   Passed    1.91 sec
          Start  341: gtest-roofit-roofit-vectorisedPDFs-testBukin
1199/3593 Test  #340: gtest-roofit-roofit-vectorisedPDFs-testLandau .....................................................   Passed    0.80 sec
          Start  342: gtest-roofit-roofit-vectorisedPDFs-testChebychev
1200/3593 Test  #341: gtest-roofit-roofit-vectorisedPDFs-testBukin ......................................................   Passed    1.27 sec
          Start  343: gtest-roofit-roofit-vectorisedPDFs-testPolynomial
1201/3593 Test  #338: gtest-roofit-roofit-vectorisedPDFs-testProductPdf .................................................   Passed    3.06 sec
          Start  344: gtest-roofit-roofit-vectorisedPDFs-testBernstein
1202/3593 Test  #339: gtest-roofit-roofit-vectorisedPDFs-testJohnson ....................................................   Passed    3.47 sec
          Start  345: gtest-roofit-roofit-vectorisedPDFs-testArgusBG
1203/3593 Test  #343: gtest-roofit-roofit-vectorisedPDFs-testPolynomial .................................................   Passed    1.00 sec
          Start  346: gtest-roofit-roofit-vectorisedPDFs-testBifurGauss
1204/3593 Test  #344: gtest-roofit-roofit-vectorisedPDFs-testBernstein ..................................................   Passed    1.18 sec
          Start  347: gtest-roofit-roofit-vectorisedPDFs-testBreitWigner
1205/3593 Test  #345: gtest-roofit-roofit-vectorisedPDFs-testArgusBG ....................................................   Passed    0.71 sec
          Start  348: gtest-roofit-roofit-vectorisedPDFs-testCBShape

[dpiparo]

Thanks for these changes. Running the tests now in the mean time.

[hageboeck]

Hello @meiyasan,
thanks for this request. I agree that it's logical to have all FindRoots methods const, since no internal state should be affected by it.
At the same time, before using mutable, I try to find another solution, so I inspected the code a bit and realised that the member fRoots doesn't "survive" across invocations, so it's actually unnecessary. (The vector is cleared on every invocation, so the values computed before don't seem to survive.)

So how about we go one step further, and remove the member entirely, return the vector of roots by value, and mark the FindRoot() methods const?

Since you discovered it, I'm happy to let you be the author of this improvement, but if you don't have the time to do it now, we could do it from the ROOT side. Let me know what you think.

[meiyasan]

Either approach (mutable or strictly const) works for me. I initially though the original implementation was intended for cache.
Thanks for your findings!

I’ll push the const update shortly per your recommendations.

Cheers, Marco

[github-actions]

Test Results

    22 files      22 suites   3d 23h 3m 42s ⏱️
 3 780 tests  3 777 ✅ 0 💤 3 ❌
81 228 runs  81 224 ✅ 0 💤 4 ❌

For more details on these failures, see this check.

Results for commit 9efbf1b.

[hageboeck]

I initially though the original implementation was intended for cache.

Indeed, on first sight, it looks like it, but nothing is actually cached, since the vector is always cleared. So maybe there was an intention for caching at some point, or simply a misunderstanding. 🙂
Anyway, removing the member and returning by value will make the class smaller and reduce complexity, so in my opinion, it is changing things for the better.