Const polynomial roots

[meiyasan]

This Pull request:

Hi @hageboeck ; This is a followup on my previous PR #20524 aiming to improve polynomial implementation. I just implemented your suggestions and deprecating internal fRoot property from polynomial header and defined it in each of the Find*Root methods .

Changes or fixes:

Removed fProp, added const property to public Find*Root methods.
Feel free to as necessary.

Checklist:

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

Tests:

I ran the ROOT battery of tests with couple of issues.

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..

[meiyasan]

sorry @hageboeck, I failed to put you as reviewer and cannot adjust.

[hageboeck]

Hello, thanks for the PR!

Please see a few comments inline. Could you also update the commit message along the lines of replacing "property" by "member" or similar?

More or less, one could use:

Allow for using several methods of Polynomial.h in const context.

- Remove the fRoots member. It doesn't serve any purpose, because it is cleared before every root computation.
- Return polynomial roots by value
- Mark affected methods const

[hageboeck]

I understand the intention, but maybe we should not use the f for "field" in this context, since it's not any more a field of the class.

[hageboeck]

@meiyasan one more thing:
After committing (or staging in git), you can already run git clang-format, so your changes are auto-formatted. Otherwise, the formatting check in the CI will fail and you will have to do it anyway. 😄

[meiyasan]

thanks! I just ran the formatter against the previous commit; removed the extra const you recommended & also renamed fRoots into roots (additionally, in GetRealRoots, I renamed roots into real_roots to avoid conflicts).

git clang-format b9d0ae5b4ee84d6d09fd9045c302b829c44edde8 -- math/mathmore/inc/Math/Polynomial.h     math/mathmore/src/Polynomial.cxx

[hageboeck]

thanks! I just ran the formatter against the previous commit; removed the extra const you recommended & also renamed fRoots into roots (additionally, in GetRealRoots, I renamed roots into real_roots to avoid conflicts).

Hello, sounds good! There must have been a small error in git, though:

• I don't see fRoots being renamed.
• We don't allow merge commits in PRs to ROOT. You will have to remove the two commits that merge master in your branch. That should be OK, because nobody will work on the same files. Once the merge commits are removed, you can apply the edits referring to fRoots, and then possibly git clang-format HEAD~ or git clang-format <root-project>/master, where you insert the name of the remote that points to root-project.

[vepadulano]

Thanks! A few comments with suggestions for minor improvements.

[vepadulano]

Suggested change

	fRoots.clear();
	fRoots.reserve(n);

These are not needed now

[vepadulano]

nitpick, consider the following perhaps a bit more idiomatic code

auto roots = FindRoots();
std::vector<double> realRoots;
realRoots.reserve(roots.size());
for (const auto &r: roots)
   if (r.imag() == 0)
      realRoots.push_back(r.real());

[vepadulano]

Now that I look at this twice, I see the for loop is stopping after fOrder iterations. Although it's not clear from the code whether the vector returned by FindRoots will have a size greater (or even smaller which would be UB!) than fOrder

[meiyasan]

well.. I agree the historical implementation sounds risky; what do you think about comparing double and strict 0 integers, shouldn't we implement an epsilon limit ?
additionally, considering we are removing std::vector::reverse() in other cases, we could also drop realRoots.reserve(roots.size()); here.

here would be my preference for simplicity purpose.

auto roots = FindRoots();
std::vector<double> realRoots;
for (const auto &r: roots)
   if (std::abs(r.imag()) < std::numeric_limits<double>::epsilon())
      realRoots.push_back(r.real());