From 9ef4fb51804dcda1de7d5673013d020ddf90de9f Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Tue, 31 Aug 2021 16:51:39 +0200 Subject: [PATCH 1/2] [RF] Change FCNGradAdapter interface to pass previous derivatives This is a follow up on commits 1e452a8 and 1e452a8, almost reverting the second commit completely. It addresses the following comments by @guitargeek made in PR #8694: 1. I think it's better if the new overloads of `DoDerivative` and `Gradient` that take also the previous gradients have a different names like `DoDerivativeWithPrevResult` and `GradientWithPrevResult` to avoid the member shadowing problem when only the original `DoDerivative` and `Gradient` functions are overridden. Then you don't have to add all these trivial implementations of the new functions in 1e452a8, and we also don't create warnings for users that are implementing these IFunctions. 2. We should avoid changing existing public interfaces if not necessary. In 1e452a8, you changed the interface of `NumericalDerivator::SetInitialGradient`, but it's not absolutely necessary. In the `RooGradMinimizerFcn`, you can just pass a `nullptr` as the first parameter to avoid the problem that you described in the message of commit 1e452a8. --- math/mathcore/inc/Fit/Chi2FCN.h | 17 ---------- math/mathcore/inc/Fit/LogLikelihoodFCN.h | 16 ---------- math/mathcore/inc/Fit/PoissonLikelihoodFCN.h | 17 ---------- math/mathcore/inc/Math/Functor.h | 31 ------------------- math/mathcore/inc/Math/IFunction.h | 8 ++--- .../inc/Math/MinimTransformFunction.h | 8 ----- math/mathcore/test/fit/testMinim.cxx | 18 ----------- math/mathmore/inc/Math/GSLNLSMinimizer.h | 17 ---------- math/mathmore/inc/Math/MultiNumGradFunction.h | 9 ------ math/minuit2/inc/Minuit2/FCNGradAdapter.h | 6 ++-- math/minuit2/inc/Minuit2/FCNGradientBase.h | 7 +++-- math/minuit2/inc/Minuit2/NumericalDerivator.h | 3 +- .../ExternalInternalGradientCalculator.cxx | 2 +- math/minuit2/src/NumericalDerivator.cxx | 3 +- math/minuit2/test/MnTutorial/Quad1F.h | 2 -- math/minuit2/test/MnTutorial/Quad4F.h | 2 -- math/minuit2/test/testMinimizer.cxx | 18 ----------- roofit/roofitcore/inc/RooGradMinimizerFcn.h | 4 +-- roofit/roofitcore/src/RooGradMinimizerFcn.cxx | 7 +++-- .../test/testRooGradMinimizerFcn.cxx | 18 +++++------ 20 files changed, 32 insertions(+), 181 deletions(-) diff --git a/math/mathcore/inc/Fit/Chi2FCN.h b/math/mathcore/inc/Fit/Chi2FCN.h index f45cb872052ea..bc72f64d680c5 100644 --- a/math/mathcore/inc/Fit/Chi2FCN.h +++ b/math/mathcore/inc/Fit/Chi2FCN.h @@ -126,14 +126,6 @@ class Chi2FCN : public BasicFCN { FitUtil::Evaluate::EvalChi2Gradient(BaseFCN::ModelFunction(), BaseFCN::Data(), x, g, fNEffPoints, fExecutionPolicy); } - /// In some cases, the gradient algorithm will use information from the previous step, these can be passed - /// in with this overload. The `previous_*` arrays can also be used to return second derivative and step size - /// so that these can be passed forward again as well at the call site, if necessary. - /// \warning This implementation just calls the two-parameter overload. - virtual void Gradient(const double *x, double *g, double */*previous_grad*/, double */*previous_g2*/, double */*previous_gstep*/) const - { - Gradient(x, g); - } /// get type of fit method function virtual typename BaseObjFunction::Type_t Type() const { return BaseObjFunction::kLeastSquare; } @@ -162,15 +154,6 @@ class Chi2FCN : public BasicFCN { Gradient(x, fGrad.data()); return fGrad[icoord]; } - /// In some cases, the derivative algorithm will use information from the previous step, these can be passed - /// in with this overload. The `previous_*` arrays can also be used to return second derivative and step size - /// so that these can be passed forward again as well at the call site, if necessary. - /// \warning This implementation just calls the two-parameter overload. - virtual double DoDerivative(const double *x, unsigned int icoord, double * /*previous_grad*/, double * /*previous_g2*/, - double * /*previous_gstep*/) const - { - return DoDerivative(x, icoord); - } mutable unsigned int fNEffPoints; // number of effective points used in the fit diff --git a/math/mathcore/inc/Fit/LogLikelihoodFCN.h b/math/mathcore/inc/Fit/LogLikelihoodFCN.h index bada8b1d49225..02b9ec1715a2b 100644 --- a/math/mathcore/inc/Fit/LogLikelihoodFCN.h +++ b/math/mathcore/inc/Fit/LogLikelihoodFCN.h @@ -129,14 +129,6 @@ class LogLikelihoodFCN : public BasicFCN { FitUtil::Evaluate::EvalLogLGradient(BaseFCN::ModelFunction(), BaseFCN::Data(), x, g, fNEffPoints, fExecutionPolicy); } - /// In some cases, the gradient algorithm will use information from the previous step, these can be passed - /// in with this overload. The `previous_*` arrays can also be used to return second derivative and step size - /// so that these can be passed forward again as well at the call site, if necessary. - /// \warning This overload just calls the two-parameter version. - virtual void Gradient(const double *x, double *g, double */*previous_grad*/, double */*previous_g2*/, double */*previous_gstep*/) const - { - Gradient(x, g); - } /// get type of fit method function virtual typename BaseObjFunction::Type_t Type() const { return BaseObjFunction::kLogLikelihood; } @@ -170,14 +162,6 @@ class LogLikelihoodFCN : public BasicFCN { Gradient(x, &fGrad[0]); return fGrad[icoord]; } - /// In some cases, the derivative algorithm will use information from the previous step, these can be passed - /// in with this overload. The `previous_*` arrays can also be used to return second derivative and step size - /// so that these can be passed forward again as well at the call site, if necessary. - virtual double DoDerivative(const double *x, unsigned int icoord, double * /*previous_grad*/, double * /*previous_g2*/, - double * /*previous_gstep*/) const - { - return DoDerivative(x, icoord); - } //data member diff --git a/math/mathcore/inc/Fit/PoissonLikelihoodFCN.h b/math/mathcore/inc/Fit/PoissonLikelihoodFCN.h index 7cc2ca3aeb71c..85d1025b0f6a9 100644 --- a/math/mathcore/inc/Fit/PoissonLikelihoodFCN.h +++ b/math/mathcore/inc/Fit/PoissonLikelihoodFCN.h @@ -130,14 +130,6 @@ class PoissonLikelihoodFCN : public BasicFCN FitUtil::Evaluate::EvalPoissonLogLGradient(BaseFCN::ModelFunction(), BaseFCN::Data(), x, g, fNEffPoints, fExecutionPolicy); } - /// In some cases, the gradient algorithm will use information from the previous step, these can be passed - /// in with this overload. The `previous_*` arrays can also be used to return second derivative and step size - /// so that these can be passed forward again as well at the call site, if necessary. - /// \warning This implementation just calls the two-parameter overload. - virtual void Gradient(const double *x, double *g, double */*previous_grad*/, double */*previous_g2*/, double */*previous_gstep*/) const - { - Gradient(x, g); - } /// get type of fit method function virtual typename BaseObjFunction::Type_t Type() const { return BaseObjFunction::kLogLikelihood; } @@ -178,15 +170,6 @@ class PoissonLikelihoodFCN : public BasicFCN Gradient(x, &fGrad[0]); return fGrad[icoord]; } - /// In some cases, the derivative algorithm will use information from the previous step, these can be passed - /// in with this overload. The `previous_*` arrays can also be used to return second derivative and step size - /// so that these can be passed forward again as well at the call site, if necessary. - /// \warning This implementation just calls the two-parameter overload. - virtual double DoDerivative(const double *x, unsigned int icoord, double * /*previous_grad*/, double * /*previous_g2*/, - double * /*previous_gstep*/) const - { - return DoDerivative(x, icoord); - } //data member diff --git a/math/mathcore/inc/Math/Functor.h b/math/mathcore/inc/Math/Functor.h index 0ac2fbc9605a6..43214aa8d2190 100644 --- a/math/mathcore/inc/Math/Functor.h +++ b/math/mathcore/inc/Math/Functor.h @@ -128,14 +128,6 @@ private : return fFunc.Derivative(x,icoord); } - // TODO: implementing this will require extending with another function signature in GradFunctor - /// \warning This overload just calls the two-parameter version. - inline double DoDerivative(const double *x, unsigned int icoord, double */*previous_grad*/, double */*previous_g2*/, - double */*previous_gstep*/) const - { - return DoDerivative(x, icoord); - } - unsigned int fDim; mutable Func fFunc; // should here be a reference and pass a non-const ref in ctor @@ -219,13 +211,6 @@ private : return fGradFunc(x, icoord); } - // TODO: implementing this will require extending with another function signature in GradFunctor - /// \warning This overload just calls the two-parameter version. - inline double DoDerivative(const double *x, unsigned int icoord, double */*previous_grad*/, double */*previous_g2*/, - double */*previous_gstep*/) const - { - return DoDerivative(x, icoord); - } unsigned int fDim; mutable Func fFunc; @@ -363,14 +348,6 @@ private : return ((*fObj).*fGradMemFn)(x,icoord); } - // TODO: implementing this will require extending with another function signature in GradFunctor - /// \warning This overload just calls the two-parameter version. - inline double DoDerivative(const double *x, unsigned int icoord, double */*previous_grad*/, double */*previous_g2*/, - double */*previous_gstep*/) const - { - return DoDerivative(x, icoord); - } - unsigned int fDim; mutable PointerToObj fObj; PointerToMemFn fMemFn; @@ -725,14 +702,6 @@ private : return fImpl->Derivative(x,icoord); } - // TODO: implementing this will require extending with another function signature in GradFunctor - /// \warning This overload just calls the two-parameter version. - inline double DoDerivative(const double *x, unsigned int icoord, double */*previous_grad*/, double */*previous_g2*/, - double */*previous_gstep*/) const - { - return DoDerivative(x, icoord); - } - std::unique_ptr fImpl; // pointer to base grad functor handler diff --git a/math/mathcore/inc/Math/IFunction.h b/math/mathcore/inc/Math/IFunction.h index 474a5aed6137d..5cb2a115c3ed7 100644 --- a/math/mathcore/inc/Math/IFunction.h +++ b/math/mathcore/inc/Math/IFunction.h @@ -221,7 +221,7 @@ namespace ROOT { T Derivative(const T *x, unsigned int icoord, T *previous_grad, T *previous_g2, T *previous_gstep) const { - return DoDerivative(x, icoord, previous_grad, previous_g2, previous_gstep); + return DoDerivativeWithPrevResult(x, icoord, previous_grad, previous_g2, previous_gstep); } /** @@ -243,8 +243,8 @@ namespace ROOT { /// In some cases, the derivative algorithm will use information from the previous step, these can be passed /// in with this overload. The `previous_*` arrays can also be used to return second derivative and step size /// so that these can be passed forward again as well at the call site, if necessary. - virtual T DoDerivative(const T *x, unsigned int icoord, T * /*previous_grad*/, T * /*previous_g2*/, - T * /*previous_gstep*/) const + virtual T DoDerivativeWithPrevResult(const T *x, unsigned int icoord, T * /*previous_grad*/, + T * /*previous_g2*/, T * /*previous_gstep*/) const { return DoDerivative(x, icoord); }; @@ -365,7 +365,7 @@ namespace ROOT { /// In some cases, the gradient algorithm will use information from the previous step, these can be passed /// in with this overload. The `previous_*` arrays can also be used to return second derivative and step size /// so that these can be passed forward again as well at the call site, if necessary. - virtual void Gradient(const T *x, T *grad, T *previous_grad, T *previous_g2, T *previous_gstep) const + virtual void GradientWithPrevResult(const T *x, T *grad, T *previous_grad, T *previous_g2, T *previous_gstep) const { unsigned int ndim = NDim(); for (unsigned int icoord = 0; icoord < ndim; ++icoord) diff --git a/math/mathcore/inc/Math/MinimTransformFunction.h b/math/mathcore/inc/Math/MinimTransformFunction.h index c8eac81b3e4d7..94d1877778326 100644 --- a/math/mathcore/inc/Math/MinimTransformFunction.h +++ b/math/mathcore/inc/Math/MinimTransformFunction.h @@ -123,14 +123,6 @@ class MinimTransformFunction : public IMultiGradFunction { //std::cout << "Derivative icoord (ext)" << fIndex[icoord] << " dtrafo " << dExtdInt << " " << deriv << std::endl; return deriv * dExtdInt; } - /// In some cases, the derivative algorithm will use information from the previous step, these can be passed - /// in with this overload. The `previous_*` arrays can also be used to return second derivative and step size - /// so that these can be passed forward again as well at the call site, if necessary. - virtual double DoDerivative(const double *x, unsigned int icoord, double * /*previous_grad*/, double * /*previous_g2*/, - double * /*previous_gstep*/) const - { - return DoDerivative(x, icoord); - } // copy constructor for this class (disable by having it private) MinimTransformFunction( const MinimTransformFunction & ) : diff --git a/math/mathcore/test/fit/testMinim.cxx b/math/mathcore/test/fit/testMinim.cxx index f0275e28eb486..27afa917ab1a1 100644 --- a/math/mathcore/test/fit/testMinim.cxx +++ b/math/mathcore/test/fit/testMinim.cxx @@ -188,10 +188,6 @@ public : } } - void Gradient(const double *x, double *g, double */*previous_grad*/, double */*previous_g2*/, double */*previous_gstep*/) const - { - Gradient(x, g); - } #ifdef USE_FDF void FdF (const double * x, double & f, double * g) const { @@ -244,11 +240,6 @@ public : Gradient(x,&g[0]); return g[i]; } - double DoDerivative(const double *x, unsigned int icoord, double * /*previous_grad*/, double * /*previous_g2*/, - double * /*previous_gstep*/) const - { - return DoDerivative(x, icoord); - } private: @@ -331,10 +322,6 @@ public : } - void Gradient(const double *x, double *g, double */*previous_grad*/, double */*previous_g2*/, double */*previous_gstep*/) const - { - Gradient(x, g); - } private: @@ -355,11 +342,6 @@ public : Gradient(x,&g[0]); return g[i]; } - double DoDerivative(const double *x, unsigned int icoord, double * /*previous_grad*/, double * /*previous_g2*/, - double * /*previous_gstep*/) const - { - return DoDerivative(x, icoord); - } void DoCalculatefi(const double * x) const { // calculate the i- element ; F(X) = Sum {fi] diff --git a/math/mathmore/inc/Math/GSLNLSMinimizer.h b/math/mathmore/inc/Math/GSLNLSMinimizer.h index 8bc2a9d0f311a..46cd3364c5bb2 100644 --- a/math/mathmore/inc/Math/GSLNLSMinimizer.h +++ b/math/mathmore/inc/Math/GSLNLSMinimizer.h @@ -102,14 +102,6 @@ class LSResidualFunc : public IMultiGradFunction { double f0 = 0; FdF(x,f0,g); } - /// In some cases, the gradient algorithm will use information from the previous step, these can be passed - /// in with this overload. The `previous_*` arrays can also be used to return second derivative and step size - /// so that these can be passed forward again as well at the call site, if necessary. - /// \warning This implementation just calls the two-parameter overload. - virtual void Gradient(const double *x, double *g, double */*previous_grad*/, double */*previous_g2*/, double */*previous_gstep*/) const - { - Gradient(x, g); - } void FdF (const double * x, double & f, double * g) const { unsigned int n = NDim(); @@ -137,15 +129,6 @@ class LSResidualFunc : public IMultiGradFunction { fX2[icoord] += kEps; return ( DoEval(&fX2.front()) - DoEval(x) )/kEps; } - /// In some cases, the derivative algorithm will use information from the previous step, these can be passed - /// in with this overload. The `previous_*` arrays can also be used to return second derivative and step size - /// so that these can be passed forward again as well at the call site, if necessary. - /// \warning This implementation just calls the two-parameter overload. - virtual double DoDerivative(const double *x, unsigned int icoord, double * /*previous_grad*/, double * /*previous_g2*/, - double * /*previous_gstep*/) const - { - return DoDerivative(x, icoord); - } unsigned int fIndex; const ROOT::Math::FitMethodFunction * fChi2; diff --git a/math/mathmore/inc/Math/MultiNumGradFunction.h b/math/mathmore/inc/Math/MultiNumGradFunction.h index 1676661fdbde8..2bec540179222 100644 --- a/math/mathmore/inc/Math/MultiNumGradFunction.h +++ b/math/mathmore/inc/Math/MultiNumGradFunction.h @@ -122,15 +122,6 @@ class MultiNumGradFunction : public IMultiGradFunction { // calculate derivative using mathcore derivator double DoDerivative (const double * x, unsigned int icoord ) const; - /// In some cases, the derivative algorithm will use information from the previous step, these can be passed - /// in with this overload. The `previous_*` arrays can also be used to return second derivative and step size - /// so that these can be passed forward again as well at the call site, if necessary. - /// \warning This implementation just calls the two-parameter overload. - virtual double DoDerivative(const double *x, unsigned int icoord, double * /*previous_grad*/, double * /*previous_g2*/, - double * /*previous_gstep*/) const - { - return DoDerivative(x, icoord); - } // adapat internal function type to IMultiGenFunction needed by derivative calculation const IMultiGenFunction * fFunc; diff --git a/math/minuit2/inc/Minuit2/FCNGradAdapter.h b/math/minuit2/inc/Minuit2/FCNGradAdapter.h index 5e186918da0c9..7de80a2e95ef6 100644 --- a/math/minuit2/inc/Minuit2/FCNGradAdapter.h +++ b/math/minuit2/inc/Minuit2/FCNGradAdapter.h @@ -54,10 +54,10 @@ class FCNGradAdapter : public FCNGradientBase { }); return fGrad; } - std::vector Gradient(const std::vector &v, double *previous_grad, double *previous_g2, - double *previous_gstep) const override + std::vector GradientWithPrevResult(const std::vector &v, double *previous_grad, double *previous_g2, + double *previous_gstep) const override { - fFunc.Gradient(&v[0], &fGrad[0], previous_grad, previous_g2, previous_gstep); + fFunc.GradientWithPrevResult(&v[0], &fGrad[0], previous_grad, previous_g2, previous_gstep); MnPrint("FCNGradAdapter").Debug([&](std::ostream &os) { os << "gradient in FCNAdapter = {"; diff --git a/math/minuit2/inc/Minuit2/FCNGradientBase.h b/math/minuit2/inc/Minuit2/FCNGradientBase.h index cc7259fd57dbc..14bf7b8c189b4 100644 --- a/math/minuit2/inc/Minuit2/FCNGradientBase.h +++ b/math/minuit2/inc/Minuit2/FCNGradientBase.h @@ -41,8 +41,11 @@ class FCNGradientBase : public FCNBase { virtual ~FCNGradientBase() {} virtual std::vector Gradient(const std::vector &) const = 0; - virtual std::vector Gradient(const std::vector ¶meters, double */*previous_grad*/, double */*previous_g2*/, - double */*previous_gstep*/) const { return Gradient(parameters); }; + virtual std::vector GradientWithPrevResult(const std::vector ¶meters, double * /*previous_grad*/, + double * /*previous_g2*/, double * /*previous_gstep*/) const + { + return Gradient(parameters); + }; virtual bool CheckGradient() const { return true; } diff --git a/math/minuit2/inc/Minuit2/NumericalDerivator.h b/math/minuit2/inc/Minuit2/NumericalDerivator.h index e2c90731b68c2..ee6203274f066 100644 --- a/math/minuit2/inc/Minuit2/NumericalDerivator.h +++ b/math/minuit2/inc/Minuit2/NumericalDerivator.h @@ -70,7 +70,8 @@ class NumericalDerivator { double Ext2int(const ROOT::Fit::ParameterSettings ¶meter, double val) const; double DInt2Ext(const ROOT::Fit::ParameterSettings ¶meter, double val) const; - void SetInitialGradient(const std::vector ¶meters, + void SetInitialGradient(const ROOT::Math::IBaseFunctionMultiDim *function, + const std::vector ¶meters, std::vector &gradient); inline bool AlwaysExactlyMimicMinuit2() const { return fAlwaysExactlyMimicMinuit2; } diff --git a/math/minuit2/src/ExternalInternalGradientCalculator.cxx b/math/minuit2/src/ExternalInternalGradientCalculator.cxx index 0f93f2bc6e5e8..e451598d01cff 100644 --- a/math/minuit2/src/ExternalInternalGradientCalculator.cxx +++ b/math/minuit2/src/ExternalInternalGradientCalculator.cxx @@ -54,7 +54,7 @@ ExternalInternalGradientCalculator::operator()(const MinimumParameters &par, con std::vector previous_g2(functionGradient.G2().Data(), functionGradient.G2().Data() + functionGradient.G2().size()); std::vector previous_gstep(functionGradient.Gstep().Data(), functionGradient.Gstep().Data() + functionGradient.Gstep().size()); - std::vector grad = fGradCalc.Gradient(par_vec, previous_grad.data(), previous_g2.data(), previous_gstep.data()); + std::vector grad = fGradCalc.GradientWithPrevResult(par_vec, previous_grad.data(), previous_g2.data(), previous_gstep.data()); assert(grad.size() == fTransformation.Parameters().size()); MnAlgebraicVector v(par.Vec().size()); diff --git a/math/minuit2/src/NumericalDerivator.cxx b/math/minuit2/src/NumericalDerivator.cxx index 758cd6c8d8c28..618334d0e55a4 100644 --- a/math/minuit2/src/NumericalDerivator.cxx +++ b/math/minuit2/src/NumericalDerivator.cxx @@ -221,7 +221,8 @@ double NumericalDerivator::DInt2Ext(const ROOT::Fit::ParameterSettings ¶mete // MODIFIED: /// This function was not implemented as in Minuit2. Now it copies the behavior /// of InitialGradientCalculator. See https://github.com/roofit-dev/root/issues/10 -void NumericalDerivator::SetInitialGradient(const std::vector ¶meters, +void NumericalDerivator::SetInitialGradient(const ROOT::Math::IBaseFunctionMultiDim *, + const std::vector ¶meters, std::vector &gradient) { // set an initial gradient using some given steps diff --git a/math/minuit2/test/MnTutorial/Quad1F.h b/math/minuit2/test/MnTutorial/Quad1F.h index 276282e7ec004..ba6fabf1a93a0 100644 --- a/math/minuit2/test/MnTutorial/Quad1F.h +++ b/math/minuit2/test/MnTutorial/Quad1F.h @@ -37,8 +37,6 @@ class Quad1F : public FCNGradientBase { return (std::vector(1, 2. * x)); } - virtual std::vector Gradient(const std::vector ¶meters, double */*previous_grad*/, double */*previous_g2*/, - double */*previous_gstep*/) const { return Gradient(parameters); }; void SetErrorDef(double up) { fErrorDef = up; } diff --git a/math/minuit2/test/MnTutorial/Quad4F.h b/math/minuit2/test/MnTutorial/Quad4F.h index ed90fc4925079..1089184ca7a69 100644 --- a/math/minuit2/test/MnTutorial/Quad4F.h +++ b/math/minuit2/test/MnTutorial/Quad4F.h @@ -72,8 +72,6 @@ class Quad4FGrad : public FCNGradientBase { g[3] = 2. * w; return g; } - virtual std::vector Gradient(const std::vector ¶meters, double */*previous_grad*/, double */*previous_g2*/, - double */*previous_gstep*/) const { return Gradient(parameters); }; double Up() const { return 1.; } diff --git a/math/minuit2/test/testMinimizer.cxx b/math/minuit2/test/testMinimizer.cxx index dea116e13fd85..92f7817204b1a 100644 --- a/math/minuit2/test/testMinimizer.cxx +++ b/math/minuit2/test/testMinimizer.cxx @@ -169,10 +169,6 @@ class TrigoFletcherFunction : public ROOT::Math::IMultiGradFunction { } } } - void Gradient(const double *x, double *g, double */*previous_grad*/, double */*previous_g2*/, double */*previous_gstep*/) const - { - Gradient(x, g); - } #ifdef USE_FDF void FdF(const double *x, double &f, double *g) const @@ -224,11 +220,6 @@ class TrigoFletcherFunction : public ROOT::Math::IMultiGradFunction { Gradient(x, &g[0]); return g[i]; } - double DoDerivative(const double *x, unsigned int icoord, double * /*previous_grad*/, double * /*previous_g2*/, - double * /*previous_gstep*/) const - { - return DoDerivative(x, icoord); - } private: unsigned int fDim; @@ -295,10 +286,6 @@ class ChebyQuadFunction : public ROOT::Math::IMultiGradFunction { g[j] = 2. * g[j] / double(n); } } - void Gradient(const double *x, double *g, double */*previous_grad*/, double */*previous_g2*/, double */*previous_gstep*/) const - { - Gradient(x, g); - } private: double DoEval(const double *x) const @@ -319,11 +306,6 @@ class ChebyQuadFunction : public ROOT::Math::IMultiGradFunction { Gradient(x, &g[0]); return g[i]; } - double DoDerivative(const double *x, unsigned int icoord, double * /*previous_grad*/, double * /*previous_g2*/, - double * /*previous_gstep*/) const - { - return DoDerivative(x, icoord); - } void DoCalculatefi(const double *x) const { diff --git a/roofit/roofitcore/inc/RooGradMinimizerFcn.h b/roofit/roofitcore/inc/RooGradMinimizerFcn.h index f17563b60e0f6..4f9e2edbb3af6 100644 --- a/roofit/roofitcore/inc/RooGradMinimizerFcn.h +++ b/roofit/roofitcore/inc/RooGradMinimizerFcn.h @@ -70,8 +70,8 @@ class RooGradMinimizerFcn : public ROOT::Math::IMultiGradFunction, public RooAbs // IMultiGradFunction overrides double DoEval(const double *x) const override; double DoDerivative(const double *x, unsigned int icoord) const override; - double DoDerivative(const double *x, unsigned int i_component, double *previous_grad, - double *previous_g2, double *previous_gstep) const override; + double DoDerivativeWithPrevResult(const double *x, unsigned int i_component, double *previous_grad, + double *previous_g2, double *previous_gstep) const override; // members // mutable because ROOT::Math::IMultiGradFunction::DoDerivative is const diff --git a/roofit/roofitcore/src/RooGradMinimizerFcn.cxx b/roofit/roofitcore/src/RooGradMinimizerFcn.cxx index 6e389a1d555ac..382b41ce1bb3c 100644 --- a/roofit/roofitcore/src/RooGradMinimizerFcn.cxx +++ b/roofit/roofitcore/src/RooGradMinimizerFcn.cxx @@ -64,7 +64,7 @@ ROOT::Math::IMultiGradFunction *RooGradMinimizerFcn::Clone() const void RooGradMinimizerFcn::synchronizeGradientParameterSettings( std::vector ¶meter_settings) const { - _gradf.SetInitialGradient(parameter_settings, _grad); + _gradf.SetInitialGradient(nullptr, parameter_settings, _grad); } //////////////////////////////////////////////////////////////////////////////// @@ -215,8 +215,9 @@ double RooGradMinimizerFcn::DoDerivative(const double *x, unsigned int i_compone return _grad[i_component].derivative; } -double RooGradMinimizerFcn::DoDerivative(const double *x, unsigned int i_component, double *previous_grad, - double *previous_g2, double *previous_gstep) const +double RooGradMinimizerFcn::DoDerivativeWithPrevResult(const double *x, unsigned int i_component, + double *previous_grad, double *previous_g2, + double *previous_gstep) const { syncParameters(x); _grad[i_component] = {previous_grad[i_component], previous_g2[i_component], previous_gstep[i_component]}; diff --git a/roofit/roofitcore/test/testRooGradMinimizerFcn.cxx b/roofit/roofitcore/test/testRooGradMinimizerFcn.cxx index d1ee53aec994e..e3cccab39a939 100644 --- a/roofit/roofitcore/test/testRooGradMinimizerFcn.cxx +++ b/roofit/roofitcore/test/testRooGradMinimizerFcn.cxx @@ -72,7 +72,7 @@ TEST_P(GradMinimizerParSeed, Gaussian1D) *values = *savedValues; - std::unique_ptr m1 = RooMinimizer::create(*nll, RooMinimizer::FcnMode::gradient); + std::unique_ptr m1 = RooMinimizer::create(*nll); m1->setMinimizerType("Minuit2"); m1->setStrategy(0); @@ -186,8 +186,8 @@ TEST(GradMinimizer, GaussianND) RooFitResult *m0result = m0.lastMinuitFit(); double minNll0 = m0result->minNll(); double edm0 = m0result->edm(); - std::vector mean0(N); - std::vector std0(N); + double mean0[N]; + double std0[N]; for (unsigned ix = 0; ix < N; ++ix) { { std::ostringstream os; @@ -218,8 +218,8 @@ TEST(GradMinimizer, GaussianND) RooFitResult *m1result = m1->lastMinuitFit(); double minNll1 = m1result->minNll(); double edm1 = m1result->edm(); - std::vector mean1(N); - std::vector std1(N); + double mean1[N]; + double std1[N]; for (unsigned ix = 0; ix < N; ++ix) { { std::ostringstream os; @@ -283,8 +283,8 @@ TEST(GradMinimizerReverse, GaussianND) RooFitResult *m0result = m0->lastMinuitFit(); double minNll0 = m0result->minNll(); double edm0 = m0result->edm(); - std::vector mean0(N); - std::vector std0(N); + double mean0[N]; + double std0[N]; for (unsigned ix = 0; ix < N; ++ix) { { std::ostringstream os; @@ -315,8 +315,8 @@ TEST(GradMinimizerReverse, GaussianND) RooFitResult *m1result = m1.lastMinuitFit(); double minNll1 = m1result->minNll(); double edm1 = m1result->edm(); - std::vector mean1(N); - std::vector std1(N); + double mean1[N]; + double std1[N]; for (unsigned ix = 0; ix < N; ++ix) { { std::ostringstream os; From fd28c8f5b27997363785f3281f100d725afb7e24 Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Tue, 31 Aug 2021 17:45:50 +0200 Subject: [PATCH 2/2] [RF] Speedup GradMinimizer.BranchingPDF test in testRooGradMinimizerFcn The speedup is achieved by reducing the number of observables in the model, meaning less numeric integration is necessary. This was done because originally, the GradMinimizer.BranchingPDF test took between 3 and 5 minutes to run, depending on the setup. --- .../test/testRooGradMinimizerFcn.cxx | 44 +++++++++---------- 1 file changed, 20 insertions(+), 24 deletions(-) diff --git a/roofit/roofitcore/test/testRooGradMinimizerFcn.cxx b/roofit/roofitcore/test/testRooGradMinimizerFcn.cxx index e3cccab39a939..e393070f34653 100644 --- a/roofit/roofitcore/test/testRooGradMinimizerFcn.cxx +++ b/roofit/roofitcore/test/testRooGradMinimizerFcn.cxx @@ -72,7 +72,7 @@ TEST_P(GradMinimizerParSeed, Gaussian1D) *values = *savedValues; - std::unique_ptr m1 = RooMinimizer::create(*nll); + std::unique_ptr m1 = RooMinimizer::create(*nll, RooMinimizer::FcnMode::gradient); m1->setMinimizerType("Minuit2"); m1->setStrategy(0); @@ -186,8 +186,8 @@ TEST(GradMinimizer, GaussianND) RooFitResult *m0result = m0.lastMinuitFit(); double minNll0 = m0result->minNll(); double edm0 = m0result->edm(); - double mean0[N]; - double std0[N]; + std::vector mean0(N); + std::vector std0(N); for (unsigned ix = 0; ix < N; ++ix) { { std::ostringstream os; @@ -218,8 +218,8 @@ TEST(GradMinimizer, GaussianND) RooFitResult *m1result = m1->lastMinuitFit(); double minNll1 = m1result->minNll(); double edm1 = m1result->edm(); - double mean1[N]; - double std1[N]; + std::vector mean1(N); + std::vector std1(N); for (unsigned ix = 0; ix < N; ++ix) { { std::ostringstream os; @@ -283,8 +283,8 @@ TEST(GradMinimizerReverse, GaussianND) RooFitResult *m0result = m0->lastMinuitFit(); double minNll0 = m0result->minNll(); double edm0 = m0result->edm(); - double mean0[N]; - double std0[N]; + std::vector mean0(N); + std::vector std0(N); for (unsigned ix = 0; ix < N; ++ix) { { std::ostringstream os; @@ -315,8 +315,8 @@ TEST(GradMinimizerReverse, GaussianND) RooFitResult *m1result = m1.lastMinuitFit(); double minNll1 = m1result->minNll(); double edm1 = m1result->edm(); - double mean1[N]; - double std1[N]; + std::vector mean1(N); + std::vector std1(N); for (unsigned ix = 0; ix < N; ++ix) { { std::ostringstream os; @@ -350,7 +350,7 @@ TEST(GradMinimizer, BranchingPDF) // produce the same random stuff every time RooRandom::randomGenerator()->SetSeed(1); - RooWorkspace w("w", kFALSE); + RooWorkspace w("w", false); // 3rd level w.factory("Gamma::ga0_0_1(k0_0_1[3,2,10],u[1,20],1,0)"); // leaf pdf @@ -369,9 +369,7 @@ TEST(GradMinimizer, BranchingPDF) // 1st level w.factory("Gaussian::g0(x[-10,10],g0_0,s0[3,0.1,10])"); // branch pdf w.factory("Gaussian::g1(y[-10,10],m1[-2,-10,10],ga1_0)"); // branch pdf - RooArgSet level1_pdfs; - level1_pdfs.add(*w.arg("g0")); - level1_pdfs.add(*w.arg("g1")); + RooArgSet level1_pdfs{*w.arg("g0"), *w.arg("g1")}; // event counts for 1st level pdfs RooRealVar N_g0("N_g0", "#events g0", N_events / 10, 0., 10 * N_events); @@ -379,43 +377,41 @@ TEST(GradMinimizer, BranchingPDF) w.import(N_g0); w.import(N_g1); // gather in count_set - RooArgSet level1_counts; - level1_counts.add(N_g0); - level1_counts.add(N_g1); + RooArgSet level1_counts{N_g0, N_g1}; // finally, sum the top level pdfs RooAddPdf sum("sum", "gaussian tree", level1_pdfs, level1_counts); // gather observables RooArgSet obs_set; - for (auto obs : {"x", "y", "z", "u", "v"}) { + for (auto obs : {"x", "y", "v"}) { obs_set.add(*w.arg(obs)); } // --- Generate a toyMC sample from composite PDF --- - RooDataSet *data = sum.generate(obs_set, N_events); + std::unique_ptr data{sum.generate(obs_set, N_events)}; auto nll = sum.createNLL(*data); // gather all values of parameters, observables, pdfs and nll here for easy // saving and restoring - RooArgSet some_values = RooArgSet(obs_set, w.allPdfs(), "some_values"); - RooArgSet most_values = RooArgSet(some_values, level1_counts, "most_values"); + RooArgSet some_values{obs_set, w.allPdfs(), "some_values"}; + RooArgSet most_values{some_values, level1_counts, "most_values"}; most_values.add(*nll); most_values.add(sum); - RooArgSet *param_set = nll->getParameters(obs_set); + std::unique_ptr param_set{nll->getParameters(obs_set)}; - RooArgSet all_values = RooArgSet(most_values, *param_set, "all_values"); + RooArgSet all_values{most_values, *param_set, "all_values"}; // set parameter values randomly so that they actually need to do some fitting auto it = all_values.fwdIterator(); - while (RooRealVar *val = dynamic_cast(it.next())) { + while (auto *val = dynamic_cast(it.next())) { val->setVal(RooRandom::randomGenerator()->Uniform(val->getMin(), val->getMax())); } // save initial values for the start of all minimizations - RooArgSet *savedValues = dynamic_cast(all_values.snapshot()); + std::unique_ptr savedValues{static_cast(all_values.snapshot())}; if (savedValues == nullptr) { throw std::runtime_error("params->snapshot() cannot be casted to RooArgSet!"); }