From 420aa7d661a48901fba5023c9e2f0947aef6d9e4 Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Sun, 25 Feb 2024 05:17:15 +0100 Subject: [PATCH 1/2] [RF] Consistent treatment of empty datasets with new CPU backend As we discovered in a CMSSW ROOT `master` sync PR, the new RooFit CPU backend treats empty datasets differently from the legacy NLL evaluation backend: https://github.com/cms-sw/cmsdist/pull/9025 This commit is fixing this, in particular removing the assumption that datasets used for fits with the new CPU backend are never empty. A unit test that validates the behavior for empty data objects is also added. --- roofit/batchcompute/src/RooBatchCompute.cu | 16 +++- roofit/batchcompute/src/RooBatchCompute.cxx | 18 ++--- .../inc/RooFit/Detail/BatchModeDataHelpers.h | 3 +- .../roofitcore/src/BatchModeDataHelpers.cxx | 23 +++--- roofit/roofitcore/src/RooDataSet.cxx | 2 +- roofit/roofitcore/src/RooFit/Evaluator.cxx | 39 +++++----- roofit/roofitcore/src/RooFuncWrapper.cxx | 6 +- roofit/roofitcore/test/testTestStatistics.cxx | 73 ++++++++++++++++--- 8 files changed, 120 insertions(+), 60 deletions(-) diff --git a/roofit/batchcompute/src/RooBatchCompute.cu b/roofit/batchcompute/src/RooBatchCompute.cu index 34bfe5885fdd3..55f3d4d457b1c 100644 --- a/roofit/batchcompute/src/RooBatchCompute.cu +++ b/roofit/batchcompute/src/RooBatchCompute.cu @@ -47,14 +47,19 @@ void fillBatches(Batches &batches, RestrictArr output, size_t nEvents, std::size batches._output = output; } -void fillArrays(Batch *arrays, const VarVector &vars, double *buffer, double *bufferDevice) +void fillArrays(Batch *arrays, const VarVector &vars, double *buffer, double *bufferDevice, std::size_t nEvents) { for (int i = 0; i < vars.size(); i++) { const std::span &span = vars[i]; - if (span.size() == 1) { + if (!span.empty() && span.size() < nEvents) { + // In the scalar case, the value is not on the GPU yet, so we have to + // copy the value to the GPU buffer. buffer[i] = span[0]; arrays[i].set(bufferDevice + i, false); } else { + // In the vector input cases, they are already on the GPU, so we can + // fill be buffer with some dummy value and set the input span + // directly. buffer[i] = 0.0; arrays[i].set(span.data(), true); } @@ -132,7 +137,7 @@ public: auto extraArgsDevice = reinterpret_cast(scalarBufferDevice + vars.size()); fillBatches(*batches, output, nEvents, vars.size(), extraArgs.size()); - fillArrays(arrays, vars, scalarBuffer, scalarBufferDevice); + fillArrays(arrays, vars, scalarBuffer, scalarBufferDevice, nEvents); batches->_arrays = arraysDevice; if (!extraArgs.empty()) { @@ -255,6 +260,8 @@ __global__ void nllSumKernel(const double *__restrict__ probas, const double *__ double RooBatchComputeClass::reduceSum(RooBatchCompute::Config const &cfg, InputArr input, size_t n) { + if (n == 0) + return 0.0; const int gridSize = getGridSize(n); cudaStream_t stream = *cfg.cudaStream(); CudaInterface::DeviceArray devOut(2 * gridSize); @@ -270,6 +277,9 @@ ReduceNLLOutput RooBatchComputeClass::reduceNLL(RooBatchCompute::Config const &c std::span weights, std::span offsetProbas) { ReduceNLLOutput out; + if (probas.empty()) { + return out; + } const int gridSize = getGridSize(probas.size()); CudaInterface::DeviceArray devOut(2 * gridSize); cudaStream_t stream = *cfg.cudaStream(); diff --git a/roofit/batchcompute/src/RooBatchCompute.cxx b/roofit/batchcompute/src/RooBatchCompute.cxx index ae9970b0f1a63..b6a22cb13ded4 100644 --- a/roofit/batchcompute/src/RooBatchCompute.cxx +++ b/roofit/batchcompute/src/RooBatchCompute.cxx @@ -50,21 +50,19 @@ void fillBatches(Batches &batches, RestrictArr output, size_t nEvents, std::size batches._output = output; } -void fillArrays(std::vector &arrays, const VarVector &vars, double *buffer) +void fillArrays(std::vector &arrays, const VarVector &vars, double *buffer, std::size_t nEvents) { arrays.resize(vars.size()); for (size_t i = 0; i < vars.size(); i++) { const std::span &span = vars[i]; - if (span.empty()) { - std::stringstream ss; - ss << "The span number " << i << " passed to Batches::Batches() is empty!"; - throw std::runtime_error(ss.str()); - } else if (span.size() > 1) { - arrays[i].set(span.data(), true); - } else { + if (!span.empty() && span.size() < nEvents) { + // In the scalar case, copy the value to each element of vector input + // buffer. std::fill_n(&buffer[i * bufferSize], bufferSize, span.data()[0]); arrays[i].set(&buffer[i * bufferSize], false); + } else { + arrays[i].set(span.data(), true); } } } @@ -131,7 +129,7 @@ class RooBatchComputeClass : public RooBatchComputeInterface { Batches batches; std::vector arrays; fillBatches(batches, output, nEventsPerThread, vars.size(), extraArgs); - fillArrays(arrays, vars, buffer.data()); + fillArrays(arrays, vars, buffer.data(), nEvents); batches._arrays = arrays.data(); batches.advance(batches.getNEvents() * idx); @@ -163,7 +161,7 @@ class RooBatchComputeClass : public RooBatchComputeInterface { Batches batches; std::vector arrays; fillBatches(batches, output, nEvents, vars.size(), extraArgs); - fillArrays(arrays, vars, buffer.data()); + fillArrays(arrays, vars, buffer.data(), nEvents); batches._arrays = arrays.data(); std::size_t events = batches.getNEvents(); diff --git a/roofit/roofitcore/inc/RooFit/Detail/BatchModeDataHelpers.h b/roofit/roofitcore/inc/RooFit/Detail/BatchModeDataHelpers.h index 34367d318cc82..b41350aaeafa1 100644 --- a/roofit/roofitcore/inc/RooFit/Detail/BatchModeDataHelpers.h +++ b/roofit/roofitcore/inc/RooFit/Detail/BatchModeDataHelpers.h @@ -41,8 +41,7 @@ getDataSpans(RooAbsData const &data, std::string const &rangeName, RooSimultaneo bool takeGlobalObservablesFromData, std::stack> &buffers); std::map -determineOutputSizes(RooAbsArg const &topNode, - std::function const &inputSizeFunc); +determineOutputSizes(RooAbsArg const &topNode, std::function const &inputSizeFunc); } // namespace BatchModeDataHelpers } // namespace Detail diff --git a/roofit/roofitcore/src/BatchModeDataHelpers.cxx b/roofit/roofitcore/src/BatchModeDataHelpers.cxx index da1af654f230e..19c99a95761a1 100644 --- a/roofit/roofitcore/src/BatchModeDataHelpers.cxx +++ b/roofit/roofitcore/src/BatchModeDataHelpers.cxx @@ -54,12 +54,6 @@ getSingleDataSpans(RooAbsData const &data, std::string_view rangeName, std::stri std::size_t nEvents = static_cast(data.numEntries()); - // We also want to support empty datasets: in this case the - // RooFitDriver::Dataset is not filled with anything. - if (nEvents == 0) { - return dataSpans; - } - auto weight = data.getWeightBatch(0, nEvents, /*sumW2=*/false); auto weightSumW2 = data.getWeightBatch(0, nEvents, /*sumW2=*/true); @@ -274,7 +268,7 @@ RooFit::Detail::BatchModeDataHelpers::getDataSpans(RooAbsData const &data, std:: /// \param[in] topNode The top node of the computation graph. /// \param[in] inputSizeFunc A function to get the input sizes. std::map RooFit::Detail::BatchModeDataHelpers::determineOutputSizes( - RooAbsArg const &topNode, std::function const &inputSizeFunc) + RooAbsArg const &topNode, std::function const &inputSizeFunc) { std::map output; @@ -282,8 +276,10 @@ std::map RooFit::Detail::BatchModeDataHelp RooHelpers::getSortedComputationGraph(topNode, serverSet); for (RooAbsArg *arg : serverSet) { - std::size_t inputSize = inputSizeFunc(arg); - if (inputSize > 0) { + int inputSize = inputSizeFunc(arg); + // The size == -1 encodes that the input doesn't come from an array + // input. + if (inputSize != -1) { output[arg] = inputSize; } } @@ -296,7 +292,14 @@ std::map RooFit::Detail::BatchModeDataHelp if (!arg->isReducerNode()) { for (RooAbsArg *server : arg->servers()) { if (server->isValueServer(*arg)) { - size = std::max(output.at(server), size); + std::size_t inputSize = output.at(server); + if (inputSize != 1) { + // If the input if from an external array, the output will + // adopt its size and we can stop the checking of other + // servers. + size = inputSize; + break; + } } } } diff --git a/roofit/roofitcore/src/RooDataSet.cxx b/roofit/roofitcore/src/RooDataSet.cxx index 03507c431c444..434a9c0e98141 100644 --- a/roofit/roofitcore/src/RooDataSet.cxx +++ b/roofit/roofitcore/src/RooDataSet.cxx @@ -919,7 +919,7 @@ std::span RooDataSet::getWeightBatch(std::size_t first, std::size_ std::size_t nEntries = this->numEntries(); // for the casting to std::size_t - if(first >= nEntries || (first + len) > nEntries) { + if(first + len > nEntries) { throw std::runtime_error("RooDataSet::getWeightBatch(): requested range not valid for dataset."); } diff --git a/roofit/roofitcore/src/RooFit/Evaluator.cxx b/roofit/roofitcore/src/RooFit/Evaluator.cxx index 13a7eab674d63..d7734c6edfbfa 100644 --- a/roofit/roofitcore/src/RooFit/Evaluator.cxx +++ b/roofit/roofitcore/src/RooFit/Evaluator.cxx @@ -262,30 +262,27 @@ void Evaluator::setInput(std::string const &name, std::span inputA info.outputSize = inputArray.size(); if (_useGPU) { #ifdef ROOFIT_CUDA - if (info.outputSize == 1) { - // Scalar observables from the data don't need to be copied to the GPU + if (info.outputSize <= 1) { + // Empty or scalar observables from the data don't need to be + // copied to the GPU. _dataMapCPU.set(info.absArg, inputArray); _dataMapCUDA.set(info.absArg, inputArray); } else { - if (_useGPU) { - // For simplicity, we put the data on both host and device for - // now. This could be optimized by inspecting the clients of the - // variable. - if (isOnDevice) { - _dataMapCUDA.set(info.absArg, inputArray); - auto gpuSpan = _dataMapCUDA.at(info.absArg); - info.buffer = _bufferManager->makeCpuBuffer(gpuSpan.size()); - CudaInterface::copyDeviceToHost(gpuSpan.data(), info.buffer->cpuWritePtr(), gpuSpan.size()); - _dataMapCPU.set(info.absArg, {info.buffer->cpuReadPtr(), gpuSpan.size()}); - } else { - _dataMapCPU.set(info.absArg, inputArray); - auto cpuSpan = _dataMapCPU.at(info.absArg); - info.buffer = _bufferManager->makeGpuBuffer(cpuSpan.size()); - CudaInterface::copyHostToDevice(cpuSpan.data(), info.buffer->gpuWritePtr(), cpuSpan.size()); - _dataMapCUDA.set(info.absArg, {info.buffer->gpuReadPtr(), cpuSpan.size()}); - } + // For simplicity, we put the data on both host and device for + // now. This could be optimized by inspecting the clients of the + // variable. + if (isOnDevice) { + _dataMapCUDA.set(info.absArg, inputArray); + auto gpuSpan = _dataMapCUDA.at(info.absArg); + info.buffer = _bufferManager->makeCpuBuffer(gpuSpan.size()); + CudaInterface::copyDeviceToHost(gpuSpan.data(), info.buffer->cpuWritePtr(), gpuSpan.size()); + _dataMapCPU.set(info.absArg, {info.buffer->cpuReadPtr(), gpuSpan.size()}); } else { _dataMapCPU.set(info.absArg, inputArray); + auto cpuSpan = _dataMapCPU.at(info.absArg); + info.buffer = _bufferManager->makeGpuBuffer(cpuSpan.size()); + CudaInterface::copyHostToDevice(cpuSpan.data(), info.buffer->gpuWritePtr(), cpuSpan.size()); + _dataMapCUDA.set(info.absArg, {info.buffer->gpuReadPtr(), cpuSpan.size()}); } } #endif @@ -313,9 +310,9 @@ void Evaluator::updateOutputSizes() } auto outputSizeMap = - RooFit::Detail::BatchModeDataHelpers::determineOutputSizes(_topNode, [&](RooFit::Detail::DataKey key) { + RooFit::Detail::BatchModeDataHelpers::determineOutputSizes(_topNode, [&](RooFit::Detail::DataKey key) -> int { auto found = sizeMap.find(key); - return found != sizeMap.end() ? found->second : 0; + return found != sizeMap.end() ? found->second : -1; }); for (auto &info : _nodes) { diff --git a/roofit/roofitcore/src/RooFuncWrapper.cxx b/roofit/roofitcore/src/RooFuncWrapper.cxx index 29e89375d815d..00365ba285c24 100644 --- a/roofit/roofitcore/src/RooFuncWrapper.cxx +++ b/roofit/roofitcore/src/RooFuncWrapper.cxx @@ -100,10 +100,10 @@ void RooFuncWrapper::loadParamsAndData(RooAbsArg const *head, RooArgSet const &p _gradientVarBuffer.resize(_params.size()); if (head) { - _nodeOutputSizes = - RooFit::Detail::BatchModeDataHelpers::determineOutputSizes(*head, [&spans](RooFit::Detail::DataKey key) { + _nodeOutputSizes = RooFit::Detail::BatchModeDataHelpers::determineOutputSizes( + *head, [&spans](RooFit::Detail::DataKey key) -> int { auto found = spans.find(key); - return found != spans.end() ? found->second.size() : 0; + return found != spans.end() ? found->second.size() : -1; }); } } diff --git a/roofit/roofitcore/test/testTestStatistics.cxx b/roofit/roofitcore/test/testTestStatistics.cxx index 5bf655a8af804..2ef4082c3a862 100644 --- a/roofit/roofitcore/test/testTestStatistics.cxx +++ b/roofit/roofitcore/test/testTestStatistics.cxx @@ -24,6 +24,8 @@ #include #include +#include + #include "gtest_wrapper.h" #include @@ -269,6 +271,57 @@ TEST_P(TestStatisticTest, IntegrateBins_RooDataHist) << "Expect chi2/ndf at least 10% better."; } +// Verify that fitting an empty RooDataSet or a RooDataHist with only empty +// bins does not do anything to the parameters. The point of this test is to +// validate that the new CPU backend behaves the same as the legacy evaluation +// backend for empty data objects. +TEST_P(TestStatisticTest, EmptyData) +{ + RooWorkspace ws; + ws.factory("Gaussian::model(x[0, 10], mean[6, 0, 10], sigma[2.0, 0.01, 10.0])"); + + RooRealVar &x = *ws.var("x"); + RooRealVar &mean = *ws.var("mean"); + RooRealVar &sigma = *ws.var("sigma"); + RooAbsPdf &model = *ws.pdf("model"); + + const double meanOrigVal = mean.getVal(); + const double sigmaOrigVal = sigma.getVal(); + + std::unique_ptr data{model.generate(x, 0)}; + std::unique_ptr dataHist{data->binnedClone()}; + + { + // We expect errors in the Hessian calculation because the likelihood is + // constant. + ROOT::TestSupport::CheckDiagsRAII checkDiag; + checkDiag.requiredDiag(kWarning, "ROOT::Math::Fitter::CalculateHessErrors", "Error when calculating Hessian"); + + model.fitTo(*data, _evalBackend, RooFit::PrintLevel(-1)); + } + + EXPECT_EQ(mean.getVal(), meanOrigVal) << "Fitting an empty RooDataSet changed \"mean\" value!"; + EXPECT_EQ(sigma.getVal(), sigmaOrigVal) << "Fitting an empty RooDataSet changed \"sigma\" value!"; + + // Reset the parameters for the check with the RooDataHist + mean.setVal(meanOrigVal); + sigma.setVal(sigmaOrigVal); + mean.setError(0.0); + sigma.setError(0.0); + + { + // We expect errors in the Hessian calculation because the likelihood is + // constant (same as above for the RooDataSet). + ROOT::TestSupport::CheckDiagsRAII checkDiag; + checkDiag.requiredDiag(kWarning, "ROOT::Math::Fitter::CalculateHessErrors", "Error when calculating Hessian"); + + model.fitTo(*dataHist, _evalBackend, RooFit::PrintLevel(-1)); + } + + EXPECT_EQ(mean.getVal(), meanOrigVal) << "Fitting an empty RooDataSet changed \"mean\" value!"; + EXPECT_EQ(sigma.getVal(), sigmaOrigVal) << "Fitting an empty RooDataSet changed \"sigma\" value!"; +} + TEST(RooChi2Var, IntegrateBins) { RooHelpers::LocalChangeMsgLevel changeMsgLvl(RooFit::WARNING); @@ -587,11 +640,11 @@ INSTANTIATE_TEST_SUITE_P(RooNLLVar, TestStatisticTest, testing::Values(ROOFIT_EV INSTANTIATE_TEST_SUITE_P(RooNLLVar, OffsetBinTest, testing::Combine(testing::Values(ROOFIT_EVAL_BACKENDS), // EvalBackend - testing::Values(false, true), // unbinned or binned - testing::Values(false, true), // extended fit - testing::Values(false, true), // use sumW2 - testing::Values(false, true), // wrap in a RooSimultaneous - testing::Values(false) // binned likelihood code path + testing::Values(false, true), // unbinned or binned + testing::Values(false, true), // extended fit + testing::Values(false, true), // use sumW2 + testing::Values(false, true), // wrap in a RooSimultaneous + testing::Values(false) // binned likelihood code path ), [](testing::TestParamInfo const ¶mInfo) { std::stringstream ss; @@ -606,11 +659,11 @@ INSTANTIATE_TEST_SUITE_P(RooNLLVar, OffsetBinTest, INSTANTIATE_TEST_SUITE_P(RooNLLVarBinnedL, OffsetBinTest, testing::Combine(testing::Values(ROOFIT_EVAL_BACKENDS), // EvalBackend - testing::Values(true), // unbinned or binned - testing::Values(false), // extended fit - testing::Values(false), // use sumW2 - testing::Values(false, true), // wrap in a RooSimultaneous - testing::Values(true) // binned likelihood code path + testing::Values(true), // unbinned or binned + testing::Values(false), // extended fit + testing::Values(false), // use sumW2 + testing::Values(false, true), // wrap in a RooSimultaneous + testing::Values(true) // binned likelihood code path ), [](testing::TestParamInfo const ¶mInfo) { std::stringstream ss; From e54aecfcedf976d59653eb270dd0e20b53ccd5eb Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Sun, 25 Feb 2024 05:28:41 +0100 Subject: [PATCH 2/2] [RF] Fix some typos in RooFit Also, write out "constructor" instead of using the slang abbriviation "ctor". --- roofit/roofitcore/inc/RooAddModel.h | 2 +- roofit/roofitcore/inc/RooAddPdf.h | 2 +- roofit/roofitcore/inc/RooCompositeDataStore.h | 2 +- roofit/roofitcore/inc/RooCustomizer.h | 2 +- roofit/roofitcore/inc/RooLinkedListElem.h | 2 +- roofit/roofitcore/inc/RooMinimizer.h | 8 ++-- roofit/roofitcore/inc/RooMsgService.h | 2 +- roofit/roofitcore/inc/RooNumConvPdf.h | 2 +- roofit/roofitcore/inc/RooTreeDataStore.h | 2 +- roofit/roofitcore/inc/RooVectorDataStore.h | 2 +- roofit/roofitcore/src/RooAbsArg.cxx | 4 +- roofit/roofitcore/src/RooCustomizer.cxx | 46 +++++++++---------- roofit/roofitcore/src/RooGenProdProj.cxx | 2 +- roofit/roofitcore/src/RooHistPdf.cxx | 4 +- roofit/roofitcore/src/RooObjCacheManager.cxx | 2 +- roofit/roofitcore/src/RooPolyFunc.cxx | 2 +- roofit/roofitcore/src/RooVectorDataStore.cxx | 6 +-- roofit/roofitcore/src/RooWorkspace.cxx | 6 +-- roofit/roofitcore/test/test_lib.h | 2 +- roofit/roostats/src/BayesianCalculator.cxx | 4 +- roofit/xroofit/src/xRooNode.cxx | 6 +-- 21 files changed, 55 insertions(+), 55 deletions(-) diff --git a/roofit/roofitcore/inc/RooAddModel.h b/roofit/roofitcore/inc/RooAddModel.h index 1c95712c0ea5a..f6cd548d80c5c 100644 --- a/roofit/roofitcore/inc/RooAddModel.h +++ b/roofit/roofitcore/inc/RooAddModel.h @@ -119,7 +119,7 @@ class RooAddModel : public RooResolutionModel { RooListProxy _coefList ; ///< List of coefficients mutable RooArgList* _snormList{nullptr}; /// const& inputData) ; - // Empty ctor + // Empty constructor. RooAbsDataStore* clone(const char* newname=nullptr) const override { return new RooCompositeDataStore(*this,newname) ; } RooAbsDataStore* clone(const RooArgSet& vars, const char* newname=nullptr) const override { return new RooCompositeDataStore(*this,vars,newname) ; } diff --git a/roofit/roofitcore/inc/RooCustomizer.h b/roofit/roofitcore/inc/RooCustomizer.h index 895d87d609980..8fdd3c4a53acf 100644 --- a/roofit/roofitcore/inc/RooCustomizer.h +++ b/roofit/roofitcore/inc/RooCustomizer.h @@ -96,7 +96,7 @@ class RooCustomizer { RooArgSet _internalCloneBranchList; ///< List of branches of internal clone RooArgSet* _cloneBranchList = nullptr; ///< Pointer to list of cloned branches used - // Cloned leafs are owned by the user supplied list in the ctor + // Cloned leaves are owned by the user supplied list in the constructor RooArgSet* _cloneNodeListAll = nullptr; ///< List of all cloned nodes RooArgSet* _cloneNodeListOwned = nullptr;///< List of owned cloned nodes } ; diff --git a/roofit/roofitcore/inc/RooLinkedListElem.h b/roofit/roofitcore/inc/RooLinkedListElem.h index 5ecf1af632e25..25ffe5bc01c48 100644 --- a/roofit/roofitcore/inc/RooLinkedListElem.h +++ b/roofit/roofitcore/inc/RooLinkedListElem.h @@ -33,7 +33,7 @@ namespace RooLinkedListImplDetails { class RooLinkedListElem { public: - // Initial element ctor + // Initial element constructor RooLinkedListElem() = default; void init(TObject* arg, RooLinkedListElem* after=nullptr) { diff --git a/roofit/roofitcore/inc/RooMinimizer.h b/roofit/roofitcore/inc/RooMinimizer.h index 0792d636f1f9c..ab22d0eec2e96 100644 --- a/roofit/roofitcore/inc/RooMinimizer.h +++ b/roofit/roofitcore/inc/RooMinimizer.h @@ -42,7 +42,7 @@ class RooDataSet; class RooMinimizer : public TObject { public: - /// Config argument to RooMinimizer ctor + /// Config argument to RooMinimizer constructor. struct Config { Config() {} @@ -55,16 +55,16 @@ class RooMinimizer : public TObject { int offsetting = -1; // RooAbsMinimizerFcn config const char *logf = nullptr; // RooAbsMinimizerFcn config - // RooAbsMinimizerFcn config that can only be set in ctor, 0 means no parallelization (default), + // RooAbsMinimizerFcn config that can only be set in constructor, 0 means no parallelization (default), // -1 is parallelization with the number of workers controlled by RooFit::MultiProcess which // defaults to the number of available processors, n means parallelization with n CPU's int parallelize = 0; - // Experimental: RooAbsMinimizerFcn config that can only be set in ctor + // Experimental: RooAbsMinimizerFcn config that can only be set in constructor // argument is ignored when parallelize is 0 bool enableParallelGradient = true; - // Experimental: RooAbsMinimizerFcn config that can only be set in ctor + // Experimental: RooAbsMinimizerFcn config that can only be set in constructor // argument is ignored when parallelize is 0 bool enableParallelDescent = false; diff --git a/roofit/roofitcore/inc/RooMsgService.h b/roofit/roofitcore/inc/RooMsgService.h index 84cd567ebe9f4..1756b20639bc4 100644 --- a/roofit/roofitcore/inc/RooMsgService.h +++ b/roofit/roofitcore/inc/RooMsgService.h @@ -235,7 +235,7 @@ class RooMsgService : public TObject { Int_t _errorCount ; - // Private ctor -- singleton class + // Private constructor -- singleton class RooMsgService() ; RooMsgService(const RooMsgService&) ; diff --git a/roofit/roofitcore/inc/RooNumConvPdf.h b/roofit/roofitcore/inc/RooNumConvPdf.h index 5be484fb6d5a8..a3e655d872326 100644 --- a/roofit/roofitcore/inc/RooNumConvPdf.h +++ b/roofit/roofitcore/inc/RooNumConvPdf.h @@ -58,7 +58,7 @@ class RooNumConvPdf : public RooAbsPdf { protected: // WVE Store all properties of RooNumConvolution here so that can be take - // along in the copy ctor. + // along in the copy constructor. RooNumConvolution& conv() const { if (!_init) initialize() ; return *_conv ; } diff --git a/roofit/roofitcore/inc/RooTreeDataStore.h b/roofit/roofitcore/inc/RooTreeDataStore.h index 350ef2cba8434..2ba30d1768a31 100644 --- a/roofit/roofitcore/inc/RooTreeDataStore.h +++ b/roofit/roofitcore/inc/RooTreeDataStore.h @@ -37,7 +37,7 @@ class RooTreeDataStore : public RooAbsDataStore { RooTreeDataStore() ; RooTreeDataStore(TTree* t, const RooArgSet& vars, const char* wgtVarName=nullptr) ; - // Empty ctor + // Empty constructor RooTreeDataStore(RooStringView name, RooStringView title, const RooArgSet& vars, const char* wgtVarName=nullptr) ; RooAbsDataStore* clone(const char* newname=nullptr) const override { return new RooTreeDataStore(*this,newname) ; } RooAbsDataStore* clone(const RooArgSet& vars, const char* newname=nullptr) const override { return new RooTreeDataStore(*this,vars,newname) ; } diff --git a/roofit/roofitcore/inc/RooVectorDataStore.h b/roofit/roofitcore/inc/RooVectorDataStore.h index 948a9313b9449..c367b95dcdeca 100644 --- a/roofit/roofitcore/inc/RooVectorDataStore.h +++ b/roofit/roofitcore/inc/RooVectorDataStore.h @@ -43,7 +43,7 @@ class RooVectorDataStore : public RooAbsDataStore { RooVectorDataStore() ; - // Empty ctor + // Empty constructor RooVectorDataStore(RooStringView name, RooStringView title, const RooArgSet& vars, const char* wgtVarName=nullptr) ; RooAbsDataStore* clone(const char* newname=nullptr) const override { return new RooVectorDataStore(*this,newname) ; } diff --git a/roofit/roofitcore/src/RooAbsArg.cxx b/roofit/roofitcore/src/RooAbsArg.cxx index 9e0c5e40b041f..224d78edd234f 100644 --- a/roofit/roofitcore/src/RooAbsArg.cxx +++ b/roofit/roofitcore/src/RooAbsArg.cxx @@ -651,7 +651,7 @@ std::size_t RooAbsArg::getParametersSizeEstimate(const RooArgSet* nset) const /// ourself as top node that don't match any of the names the args in the /// supplied argset. Returns `true` only if something went wrong. /// The complement of this function is getObservables(). -/// \param[in] observables Set of leafs to ignore because they are observables and not parameters. +/// \param[in] observables Set of leaves to ignore because they are observables and not parameters. /// \param[out] outputSet Output set. /// \param[in] stripDisconnected Allow pdf to strip parameters from list before adding it. @@ -745,7 +745,7 @@ RooFit::OwningPtr RooAbsArg::getObservables(const RooArgSet* dataList /// The complement of this function is getParameters(). /// \param[in] dataList Set of leaf nodes to match. /// \param[out] outputSet Output set. -/// \param[in] valueOnly If this parameter is true, we only match leafs that +/// \param[in] valueOnly If this parameter is true, we only match leaves that /// depend on the value of any arg in `dataList`. bool RooAbsArg::getObservables(const RooAbsCollection* dataList, RooArgSet& outputSet, bool valueOnly) const diff --git a/roofit/roofitcore/src/RooCustomizer.cxx b/roofit/roofitcore/src/RooCustomizer.cxx index 2e5d612951d23..bbf9b7353cc56 100644 --- a/roofit/roofitcore/src/RooCustomizer.cxx +++ b/roofit/roofitcore/src/RooCustomizer.cxx @@ -55,8 +55,8 @@ * runblock.defineType("run1") ; * runblock.defineType("run2") ; * - * RooArgSet splitLeafs; - * RooCustomizer cust(pdf,runblock,splitLeafs); + * RooArgSet splitLeaves; + * RooCustomizer cust(pdf,runblock,splitLeaves); * cust.splitArg(alpha,runblock); * * RooAbsPdf* pdf_run1 = cust.build("run1") ; @@ -64,9 +64,9 @@ * * RooSimultaneous simpdf("simpdf","simpdf",RooArgSet(*pdf_run1,*pdf_run2)) * ``` - * If the master category state is a super category, leafs may be split + * If the master category state is a super category, leaves may be split * by any subset of that master category. E.g. if the master category - * is 'A x B', leafs may be split by A, B or AxB. + * is 'A x B', leaves may be split by A, B or AxB. * * In addition to replacing leaf nodes, RooCustomizer clones all branch * nodes that depend directly or indirectly on modified leaf nodes, so @@ -77,7 +77,7 @@ * composites are needed. * * Any leaf nodes that are created by the customizer will be put into - * the leaf list that is passed into the customizers constructor (splitLeafs in + * the leaf list that is passed into the customizers constructor (splitLeaves in * the above example. The list owner is responsible for deleting these leaf * nodes after the customizer is deleted. * @@ -86,23 +86,23 @@ * * ### Reuse nodes to customise a different PDF * By default, the customizer clones the prototype leaf node when splitting a leaf, - * but the user can feed pre-defined split leafs in leaf list. These leafs + * but the user can feed pre-defined split leaves in leaf list. These leaves * must have the name `_` to be picked up. The list - * of pre-supplied leafs may be partial, any missing split leafs will be auto + * of pre-supplied leaves may be partial, any missing split leaves will be auto * generated. * * Another common construction is to have two prototype PDFs, each to be customized * by a separate customizer instance, that share parameters. To ensure that - * the customized clones also share their respective split leafs, i.e. + * the customized clones also share their respective split leaves, i.e. * ``` * PDF1(x,y, A) and PDF2(z, A) ---> PDF1_run1(x,y, A_run1) and PDF2_run1(x,y, A_run1) * PDF1_run2(x,y, A_run2) and PDF2_run2(x,y, A_run2) * ``` * feed the same split leaf list into both customizers. In that case, the second customizer - * will pick up the split leafs instantiated by the first customizer and the link between + * will pick up the split leaves instantiated by the first customizer and the link between * the two PDFs is retained. * - * ### Customising with pre-defined leafs + * ### Customising with pre-defined leaves * If leaf nodes are provided in the sets, the customiser will use them. This is a complete * example that customises the `yield` parameter, and splits (automatically clones) the * mean of the Gaussian. This is a short version of the tutorial rf514_RooCustomizer.C. @@ -127,17 +127,17 @@ * sample.defineType("BBG2m2T"); * * - * RooArgSet customisedLeafs; - * RooArgSet allLeafs; + * RooArgSet customisedLeaves; + * RooArgSet allLeaves; * * RooRealVar mass("M", "M", 1, 0, 12000); * RooFormulaVar yield1("yieldSig_BBG1m2T","sigy1","M/3.360779",mass); * RooFormulaVar yield2("yieldSig_BBG2m2T","sigy2","M/2",mass); - * allLeafs.add(yield1); - * allLeafs.add(yield2); + * allLeaves.add(yield1); + * allLeaves.add(yield2); * * - * RooCustomizer cust(model, sample, customisedLeafs, &allLeafs); + * RooCustomizer cust(model, sample, customisedLeaves, &allLeaves); * cust.splitArg(yieldSig, sample); * cust.splitArg(meanG, sample); * @@ -206,24 +206,24 @@ Int_t init() /// replaceArg() and splitArg() functionality. /// \param[in] pdf Proto PDF to be customised. /// \param[in] masterCat Category to be used for splitting. -/// \param[in,out] splitLeafs All nodes created in +/// \param[in,out] splitLeaves All nodes created in /// the customisation process are added to this set. /// The user can provide nodes that are *taken* /// from the set if they have a name that matches `_`. /// \note The set needs to own its contents if they are user-provided. /// Use *e.g.* /// ``` -/// RooArgSet customisedLeafs; +/// RooArgSet customisedLeaves; /// auto yield1 = new RooFormulaVar("yieldSig_BBG1m2T","sigy1","M/3.360779",mass); -/// customisedLeafs.addOwned(*yield1); +/// customisedLeaves.addOwned(*yield1); /// ``` -/// \param[in,out] splitLeafsAll All leafs that are used when customising are collected here. +/// \param[in,out] splitLeavesAll All leaves that are used when customising are collected here. /// If this set already contains leaves, they will be used for customising if the names match /// as above. /// -RooCustomizer::RooCustomizer(const RooAbsArg &pdf, const RooAbsCategoryLValue &masterCat, RooArgSet &splitLeafs, - RooArgSet *splitLeafsAll) +RooCustomizer::RooCustomizer(const RooAbsArg &pdf, const RooAbsCategoryLValue &masterCat, RooArgSet &splitLeaves, + RooArgSet *splitLeavesAll) : _sterile(false), _owning(true), _masterPdf(const_cast(&pdf)), @@ -232,8 +232,8 @@ RooCustomizer::RooCustomizer(const RooAbsArg &pdf, const RooAbsCategoryLValue &m _masterLeafList("masterLeafList"), _internalCloneBranchList("cloneBranchList"), _cloneBranchList(&_internalCloneBranchList), - _cloneNodeListAll(splitLeafsAll), - _cloneNodeListOwned(&splitLeafs) + _cloneNodeListAll(splitLeavesAll), + _cloneNodeListOwned(&splitLeaves) { initialize() ; diff --git a/roofit/roofitcore/src/RooGenProdProj.cxx b/roofit/roofitcore/src/RooGenProdProj.cxx index 6d9b712d54152..7bebce149381d 100644 --- a/roofit/roofitcore/src/RooGenProdProj.cxx +++ b/roofit/roofitcore/src/RooGenProdProj.cxx @@ -55,7 +55,7 @@ RooGenProdProj::RooGenProdProj(const char *name, const char *title, const RooArg // Set expensive object cache to that of first item in prodSet setExpensiveObjectCache(_prodSet.first()->expensiveObjectCache()) ; - // Create owners of components created in ctor + // Create owners of components created in constructor _compSetOwnedN = std::make_unique(); _compSetOwnedD = std::make_unique(); diff --git a/roofit/roofitcore/src/RooHistPdf.cxx b/roofit/roofitcore/src/RooHistPdf.cxx index ac181675297e4..ca996ce745337 100644 --- a/roofit/roofitcore/src/RooHistPdf.cxx +++ b/roofit/roofitcore/src/RooHistPdf.cxx @@ -80,7 +80,7 @@ RooHistPdf::RooHistPdf(const char *name, const char *title, const RooArgSet& var // Adjust ranges of _histObsList to those of _dataHist for (const auto hobs : _histObsList) { - // Guaranteed to succeed, since checked above in ctor + // Guaranteed to succeed, since checked above in constructor RooAbsArg* dhobs = dhist.get()->find(hobs->GetName()) ; RooRealVar* dhreal = dynamic_cast(dhobs) ; if (dhreal){ @@ -135,7 +135,7 @@ RooHistPdf::RooHistPdf(const char *name, const char *title, const RooArgList& pd // Adjust ranges of _histObsList to those of _dataHist for (const auto hobs : _histObsList) { - // Guaranteed to succeed, since checked above in ctor + // Guaranteed to succeed, since checked above in constructor RooAbsArg* dhobs = dhist.get()->find(hobs->GetName()) ; RooRealVar* dhreal = dynamic_cast(dhobs) ; if (dhreal){ diff --git a/roofit/roofitcore/src/RooObjCacheManager.cxx b/roofit/roofitcore/src/RooObjCacheManager.cxx index 080f925a49a6b..95b168ef2295f 100644 --- a/roofit/roofitcore/src/RooObjCacheManager.cxx +++ b/roofit/roofitcore/src/RooObjCacheManager.cxx @@ -60,7 +60,7 @@ RooObjCacheManager::RooObjCacheManager(const RooObjCacheManager& other, RooAbsAr RooCacheManager(other,owner), _clearOnRedirect(other._clearOnRedirect), _allowOptimize(other._allowOptimize), - _optCacheModeSeen(false) // cache mode properties are not transferred in copy ctor + _optCacheModeSeen(false) // cache mode properties are not transferred in copy constructor { } diff --git a/roofit/roofitcore/src/RooPolyFunc.cxx b/roofit/roofitcore/src/RooPolyFunc.cxx index c844682721231..5873bc0a2b909 100644 --- a/roofit/roofitcore/src/RooPolyFunc.cxx +++ b/roofit/roofitcore/src/RooPolyFunc.cxx @@ -260,7 +260,7 @@ RooPolyFunc::taylorExpand(const char *name, const char *title, RooAbsReal &func, } } - // Figure out the observable values around which to exapnd + // Figure out the observable values around which to expand std::vector obsValues; if (observableValues.empty()) { obsValues.reserve(observables.size()); diff --git a/roofit/roofitcore/src/RooVectorDataStore.cxx b/roofit/roofitcore/src/RooVectorDataStore.cxx index 8f9c42236654d..1f09ea2b7dfb5 100644 --- a/roofit/roofitcore/src/RooVectorDataStore.cxx +++ b/roofit/roofitcore/src/RooVectorDataStore.cxx @@ -135,7 +135,7 @@ RooRealVar* RooVectorDataStore::weightVar(const RooArgSet& allVars, const char* //////////////////////////////////////////////////////////////////////////////// -/// Regular copy ctor +/// Regular copy constructor. RooVectorDataStore::RooVectorDataStore(const RooVectorDataStore& other, const char* newname) : RooAbsDataStore(other,newname), @@ -193,7 +193,7 @@ RooVectorDataStore::RooVectorDataStore(const RooTreeDataStore& other, const RooA //////////////////////////////////////////////////////////////////////////////// -/// Clone ctor, must connect internal storage to given new external set of vars +/// Clone constructor, must connect internal storage to given new external set of variables. RooVectorDataStore::RooVectorDataStore(const RooVectorDataStore& other, const RooArgSet& vars, const char* newname) : RooAbsDataStore(other,varsNoWeight(vars,other._wgtVar?other._wgtVar->GetName():nullptr),newname), @@ -984,7 +984,7 @@ void RooVectorDataStore::attachCache(const RooAbsArg* newOwner, const RooArgSet& // Only applicable if a cache exists if (!_cache) return ; - // Clone ctor, must connect internal storage to given new external set of vars + // Clone constructor, must connect internal storage to given new external set of variables std::vector cacheElements(_cache->realStoreList()); cacheElements.insert(cacheElements.end(), _cache->_realfStoreList.begin(), _cache->_realfStoreList.end()); diff --git a/roofit/roofitcore/src/RooWorkspace.cxx b/roofit/roofitcore/src/RooWorkspace.cxx index dd90407d5f855..4a777bb4022a5 100644 --- a/roofit/roofitcore/src/RooWorkspace.cxx +++ b/roofit/roofitcore/src/RooWorkspace.cxx @@ -1533,9 +1533,9 @@ bool RooWorkspace::CodeRepo::autoImportClass(TClass* tc, bool doReplace) return true ; } - // Require that class meets technical criteria to be persistable (i.e it has a default ctor) - // (We also need a default ctor of abstract classes, but cannot check that through is interface - // as TClass::HasDefaultCtor only returns true for callable default ctors) + // Require that class meets technical criteria to be persistable (i.e it has a default constructor) + // (We also need a default constructor of abstract classes, but cannot check that through is interface + // as TClass::HasDefaultCtor only returns true for callable default constructors) if (!(tc->Property() & kIsAbstract) && !tc->HasDefaultConstructor()) { oocoutW(_wspace,ObjectHandling) << "RooWorkspace::autoImportClass(" << _wspace->GetName() << ") WARNING cannot import class " << tc->GetName() << " : it cannot be persisted because it doesn't have a default constructor. Please fix " << endl ; diff --git a/roofit/roofitcore/test/test_lib.h b/roofit/roofitcore/test/test_lib.h index edb2af6f9620d..02a9bfc31d453 100644 --- a/roofit/roofitcore/test/test_lib.h +++ b/roofit/roofitcore/test/test_lib.h @@ -55,7 +55,7 @@ generate_1D_gaussian_pdf_nll(RooWorkspace &ws, unsigned long nEvents) } // return two unique_ptrs, the first because nll is a pointer, -// the second because RooArgSet doesn't have a move ctor +// the second because RooArgSet doesn't have a move constructor std::tuple, RooAbsPdf *, std::unique_ptr, std::unique_ptr> generate_ND_gaussian_pdf_nll(RooWorkspace &ws, unsigned int n, unsigned long nEvents, RooFit::EvalBackend evalBackend) { diff --git a/roofit/roostats/src/BayesianCalculator.cxx b/roofit/roostats/src/BayesianCalculator.cxx index 863ea42c1ea2e..4f14837549ee0 100644 --- a/roofit/roostats/src/BayesianCalculator.cxx +++ b/roofit/roostats/src/BayesianCalculator.cxx @@ -330,8 +330,8 @@ class PosteriorCdfFunction : public ROOT::Math::IGenFunction { mutable ROOT::Math::IntegratorMultiDim fIntegrator; // integrator (mutable because Integral() is not const mutable std::vector fXmin; // min value of parameters (poi+nuis) - mutable std::vector fXmax; // max value of parameters (poi+nuis) - max poi changes so it is mutable - double fNorm = 1.0; // normalization value (computed in ctor) - mutable double fNormErr = 0.0; // normalization error value (computed in ctor) + double fNorm = 1.0; // normalization value (computed in constructor) + mutable double fNormErr = 0.0; // normalization error value (computed in constructor) double fOffset = 0; // offset for computing the root double fMaxPOI = 0; // maximum value of POI bool fHasNorm = false; // flag to control first call to the function diff --git a/roofit/xroofit/src/xRooNode.cxx b/roofit/xroofit/src/xRooNode.cxx index 4971d0cd60699..c6e700668176f 100644 --- a/roofit/xroofit/src/xRooNode.cxx +++ b/roofit/xroofit/src/xRooNode.cxx @@ -5214,9 +5214,9 @@ xRooNode xRooNode::vars() const // we want // ensure all globs appear after robs, as we rely on this ordering for picking "x" var in "reduced" method xRooNode _globs; - RooArgSet allLeafs; - p->leafNodeServerList(&allLeafs); - for (auto &c : allLeafs) { + RooArgSet allLeaves; + p->leafNodeServerList(&allLeaves); + for (auto &c : allLeaves) { if (c->isFundamental() || (dynamic_cast(c) && !TString(c->GetName()).IsFloat())) { if (!c->getAttribute("global")) { out.get()->add(*c);