[Math] Avoid using std::map in Math::FitResult and fix logic

Avoid using `std::map` in Math::FitResult and fix the logic to set
whether a parameter is fixed or bound. Before, these flags were only
changed from `false` to `true`, but not the other way around, which can
also happen if the user releases a parameter in the configuration
object.

A unit test that covers this case was also implemented.

Closes #20703.

(cherry picked from commit d5085b1)

Author: guitargeek

math/mathcore/inc/Fit/FitResult.h

@@ -354,8 +354,8 @@ class FitResult {
    std::shared_ptr<ROOT::Math::IMultiGenFunction> fObjFunc; ///<! objective function used for fitting
    std::shared_ptr<IModelFunction> fFitFunc;                ///<! model function resulting  from the fit.
    std::shared_ptr<FitData>    fFitData;                    ///<! data set used in the fit
-   std::map<unsigned int, bool>           fFixedParams;     ///< list of fixed parameters
-   std::map<unsigned int, unsigned int>   fBoundParams;     ///< list of limited parameters
+   std::vector<bool>           fFixedParams;                ///< if parameters are fixed
+   std::vector<unsigned int>   fBoundParams;                ///< if parameters are limited
    std::vector<std::pair<double,double> >  fParamBounds;    ///< parameter bounds
    std::vector<double>         fParams;    ///< parameter values. Size is total number of parameters
    std::vector<double>         fErrors;    ///< errors

math/mathcore/src/FitResult.cxx

@@ -79,19 +79,19 @@ FitResult::FitResult(const FitConfig & fconfig) :
 
    // get parameter values and errors (step sizes)
    unsigned int npar = fconfig.NPar();
+   fParamBounds.resize(npar);
+   fFixedParams.resize(npar);
+   fBoundParams.resize(npar);
    for (unsigned int i = 0; i < npar; ++i ) {
       const ParameterSettings & par = fconfig.ParSettings(i);
       fParams[i]   =  par.Value();
       fErrors[i]   =  par.StepSize();
       fParNames[i] =  par.Name();
-      if (par.IsFixed() ) fFixedParams[i] = true;
-      else fNFree++;
-      if (par.IsBound() ) {
-         double lower = (par.HasLowerLimit()) ? par.LowerLimit() : - std::numeric_limits<double>::infinity() ;
-         double upper = (par.HasUpperLimit()) ? par.UpperLimit() :   std::numeric_limits<double>::infinity() ;
-         fBoundParams[i] = fParamBounds.size();
-         fParamBounds.push_back(std::make_pair(lower,upper));
-      }
+      fFixedParams[i] = par.IsFixed();
+      if (!par.IsFixed() ) fNFree++;
+      double lower = par.HasLowerLimit() ? par.LowerLimit() : - std::numeric_limits<double>::infinity() ;
+      double upper = par.HasUpperLimit() ? par.UpperLimit() :   std::numeric_limits<double>::infinity() ;
+      fParamBounds.emplace_back(lower,upper);
    }
    std::cout << "create fit result from config - nfree " << fNFree << std::endl;
 }
@@ -155,17 +155,17 @@ void FitResult::FillResult(const std::shared_ptr<ROOT::Math::Minimizer> & min, c
 
    // check for fixed or limited parameters
    unsigned int nfree = 0;
-   if (!fParamBounds.empty()) fParamBounds.clear();
+   fParamBounds.resize(npar);
+   fFixedParams.resize(npar);
+   fBoundParams.resize(npar);
    for (unsigned int ipar = 0; ipar < npar; ++ipar) {
       const ParameterSettings & par = fconfig.ParSettings(ipar);
-      if (par.IsFixed() ) fFixedParams[ipar] = true;
-      else nfree++;
-      if (par.IsBound() ) {
-         double lower = (par.HasLowerLimit()) ? par.LowerLimit() : - std::numeric_limits<double>::infinity() ;
-         double upper = (par.HasUpperLimit()) ? par.UpperLimit() :   std::numeric_limits<double>::infinity() ;
-         fBoundParams[ipar] = fParamBounds.size();
-         fParamBounds.push_back(std::make_pair(lower,upper));
-      }
+      fFixedParams[ipar] = par.IsFixed();
+      fBoundParams[ipar] = par.IsBound();
+      if (!par.IsFixed() ) nfree++;
+      double lower = par.HasLowerLimit() ? par.LowerLimit() : - std::numeric_limits<double>::infinity() ;
+      double upper = par.HasUpperLimit() ? par.UpperLimit() :   std::numeric_limits<double>::infinity() ;
+      fParamBounds.emplace_back(lower,upper);
    }
    // check if nfree (from FitConfig) and fNFree (from minimizer) are consistent
    if (nfree != fNFree ) {
@@ -343,25 +343,24 @@ int FitResult::Index(const std::string & name) const {
    return -1; // case name is not found
 }
 
-bool FitResult::IsParameterBound(unsigned int ipar) const {
-   return fBoundParams.find(ipar) != fBoundParams.end();
+bool FitResult::IsParameterBound(unsigned int ipar) const
+{
+   return ipar < fBoundParams.size() ? fBoundParams[ipar] : false;
 }
 
-bool FitResult::IsParameterFixed(unsigned int ipar) const {
-   return fFixedParams.find(ipar) != fFixedParams.end();
+bool FitResult::IsParameterFixed(unsigned int ipar) const
+{
+   return ipar < fFixedParams.size() ? fFixedParams[ipar] : false;
 }
 
-bool FitResult::ParameterBounds(unsigned int ipar, double & lower, double & upper) const {
-   std::map<unsigned int, unsigned int>::const_iterator itr =  fBoundParams.find(ipar);
-   if (itr ==  fBoundParams.end() ) {
-      lower =  -std::numeric_limits<Double_t>::infinity();
-      upper =  std::numeric_limits<Double_t>::infinity();
-      return false;
+bool FitResult::ParameterBounds(unsigned int ipar, double &lower, double &upper) const
+{
+   constexpr double inf = std::numeric_limits<double>::infinity();
+   if (ipar < fParamBounds.size()) {
+      lower = fParamBounds[ipar].first;
+      upper = fParamBounds[ipar].second;
    }
-   assert(itr->second < fParamBounds.size() );
-   lower = fParamBounds[itr->second].first;
-   upper = fParamBounds[itr->second].second;
-   return true;
+   return lower != -inf || upper != inf;
 }
 
 std::string FitResult::ParName(unsigned int ipar) const {

math/mathcore/test/CMakeLists.txt

@@ -100,3 +100,5 @@ ROOT_ADD_GTEST(testKNNDensity testKNNDensity.cxx LIBRARIES Core MathCore Hist)
 if(clad)
   ROOT_ADD_GTEST(CladDerivatorTests CladDerivatorTests.cxx LIBRARIES Core MathCore)
 endif()
+
+ROOT_ADD_GTEST(testFitter testFitter.cxx LIBRARIES Core MathCore)

math/mathcore/test/testFitter.cxx

@@ -0,0 +1,77 @@
+// Tests for the ROOT::Fit::Fitter
+
+#include <Fit/Fitter.h>
+#include <Math/Functor.h>
+
+#include <gtest/gtest.h>
+
+namespace {
+
+double gauss(double *x, double *p)
+{
+   double A = p[0];
+   double mu = p[1];
+   double sigma = p[2];
+   double xx = x[0];
+   return A * exp(-(xx - mu) * (xx - mu) / (2 * sigma * sigma));
+}
+
+} // namespace
+
+/// Check if we can fix and release parameters, and that this will be correctly
+/// reflected in the FitResult.
+/// Covers https://github.com/root-project/root/issues/20703
+TEST(Fitter, FitAndReleaseParams)
+{
+   // --- Create data ---
+   const int n = 50;
+   double x[n];
+   double y[n];
+   std::vector<double> initVals{1.0, 2.0, 1.0};
+   for (int i = 0; i < n; ++i) {
+      x[i] = i * 0.1;
+      y[i] = gauss(&x[i], initVals.data());
+   }
+
+   // --- Model functor ---
+   ROOT::Math::Functor f(
+      [&](const double *p) {
+         double chi2 = 0.0;
+         for (int i = 0; i < n; ++i) {
+            double yi = gauss(&x[i], (double *)p);
+            double diff = y[i] - yi;
+            chi2 += diff * diff;
+         }
+         return chi2;
+      },
+      3);
+
+   ROOT::Fit::Fitter fitter;
+
+   std::vector<bool> fixState{false, false, false};
+   fitter.Config().SetParamsSettings(initVals.size(), initVals.data());
+
+   fitter.Config().ParSettings(0).SetName("A");
+   fitter.Config().ParSettings(1).SetName("mu");
+   fitter.Config().ParSettings(2).SetName("sigma");
+
+   // Repeatedly run fits using the SAME fitter
+   for (int iter = 0; iter < 6; ++iter) {
+      bool fix = (iter % 2 == 0);
+      int iparam = (iter / 2) % initVals.size();
+
+      fixState[iparam] = fix;
+      if (fix) {
+         fitter.Config().ParSettings(iparam).Fix();
+      } else {
+         fitter.Config().ParSettings(iparam).Release();
+      }
+
+      fitter.FitFCN(f, nullptr, 3);
+      const ROOT::Fit::FitResult &res = fitter.Result();
+
+      for (unsigned int i = 0; i < initVals.size(); ++i) {
+         EXPECT_EQ(res.IsParameterFixed(i), fixState[i]);
+      }
+   }
+}