From 013f82473b8ac245d7461586a73e0ced0cedbc0f Mon Sep 17 00:00:00 2001 From: Massimiliano Galli Date: Mon, 31 Mar 2025 15:07:00 +0200 Subject: [PATCH 1/4] [RF] Fix implementation of HS3 importers Fix by @guitargeek. The RooWSFactoryTool expression handler was not correctly matching branckets, leading to failures when importing valid JSON filed with RooProdPdf objects. --- roofit/hs3/src/RooJSONFactoryWSTool.cxx | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/roofit/hs3/src/RooJSONFactoryWSTool.cxx b/roofit/hs3/src/RooJSONFactoryWSTool.cxx index 8d047d6afe6dd..3489b0c5b7e98 100644 --- a/roofit/hs3/src/RooJSONFactoryWSTool.cxx +++ b/roofit/hs3/src/RooJSONFactoryWSTool.cxx @@ -370,8 +370,9 @@ std::string generate(const RooFit::JSONIO::ImportExpression &ex, const JSONNode RooJSONFactoryWSTool::error(errMsg.str()); } else if (p[k].is_seq()) { bool firstInner = true; + expression << "{"; for (RooAbsArg *arg : tool->requestArgList(p, k)) { - expression << (firstInner ? "{" : ",") << arg->GetName(); + expression << (firstInner ? "" : ",") << arg->GetName(); firstInner = false; } expression << "}"; From a666eb05ce52509db9bb43967f8c4bbb571036df Mon Sep 17 00:00:00 2001 From: iarspider Date: Wed, 2 Apr 2025 09:15:30 +0200 Subject: [PATCH 2/4] SearchInstalledSoftware.cmake: correctly report name of option to disable --- cmake/modules/SearchInstalledSoftware.cmake | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmake/modules/SearchInstalledSoftware.cmake b/cmake/modules/SearchInstalledSoftware.cmake index 63ee2580349af..7878ae2f9379a 100644 --- a/cmake/modules/SearchInstalledSoftware.cmake +++ b/cmake/modules/SearchInstalledSoftware.cmake @@ -54,7 +54,7 @@ endmacro() # stop the configuration with a FATAL_ERROR in case of fail-on-missing=ON. #---------------------------------------------------------------------------- macro(ROOT_CHECK_CONNECTION_AND_DISABLE_OPTION option_name) - ROOT_CHECK_CONNECTION("$(option_name)=OFF") + ROOT_CHECK_CONNECTION("${option_name}=OFF") if(NO_CONNECTION) message(STATUS "No internet connection, disabling '${option_name}' option") set(${option_name} OFF CACHE BOOL "Disabled because there is no internet connection" FORCE) From 8810d506d9a12ab8394b53a7438ce7819e4229a5 Mon Sep 17 00:00:00 2001 From: ferdymercury Date: Wed, 2 Apr 2025 11:27:28 +0200 Subject: [PATCH 3/4] [skip-ci] cpu backend specify it's single threaded See https://github.com/root-project/root/issues/17344 --- roofit/roofitcore/src/RooAbsPdf.cxx | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/roofit/roofitcore/src/RooAbsPdf.cxx b/roofit/roofitcore/src/RooAbsPdf.cxx index 46e4eadf1c423..891ccc1e1c6ce 100644 --- a/roofit/roofitcore/src/RooAbsPdf.cxx +++ b/roofit/roofitcore/src/RooAbsPdf.cxx @@ -834,7 +834,7 @@ double RooAbsPdf::extendedTerm(RooAbsData const& data, bool weightSquared, bool * \f] * `Range(double lo, double hi)` Fit only data inside given range. A range named "fit" is created on the fly on all observables. * `SumCoefRange(const char* name)` Set the range in which to interpret the coefficients of RooAddPdf components - * `NumCPU(int num, int istrat)` Parallelize NLL calculation on num CPUs + * `NumCPU(int num, int istrat)` Parallelize NLL calculation on num CPUs. (Currently, this setting is ignored with the **cpu** Backend.) * *
Strategy Effect *
0 = RooFit::BulkPartition - *default* Divide events in N equal chunks @@ -853,10 +853,10 @@ double RooAbsPdf::extendedTerm(RooAbsData const& data, bool weightSquared, bool *
`EvalBackend(std::string const&)` Choose a likelihood evaluation backend: * *
Backend Description - *
**cpu** - *default* New vectorized evaluation mode, using faster math functions and auto-vectorisation. + *
**cpu** - *default* New vectorized evaluation mode, using faster math functions and auto-vectorisation (currently on a single thread). * Since ROOT 6.23, this is the default if `EvalBackend()` is not passed, succeeding the **legacy** backend. * If all RooAbsArg objects in the model support vectorized evaluation, - * likelihood computations are 2 to 10 times faster than with the **legacy** backend + * likelihood computations are 2 to 10 times faster than with the **legacy** backend (each on a single thread). * - unless your dataset is so small that the vectorization is not worth it. * The relative difference of the single log-likelihoods with respect to the legacy mode is usually better than \f$10^{-12}\f$, * and for fit parameters it's usually better than \f$10^{-6}\f$. In past ROOT releases, this backend could be activated with the now deprecated `BatchMode()` option. @@ -866,6 +866,7 @@ double RooAbsPdf::extendedTerm(RooAbsData const& data, bool weightSquared, bool * This backend can drastically speed up the fit if all RooAbsArg object in the model support it. *
**legacy** The original likelihood evaluation method. * Evaluates the PDF for each single data entry at a time before summing the negative log probabilities. + * It supports multi-threading, but you might need more than 20 threads to maybe see about 10% performance gain over the default cpu-backend (that runs currently only on a single thread). *
**codegen** **Experimental** - Generates and compiles minimal C++ code for the NLL on-the-fly and wraps it in the returned RooAbsReal. * Also generates and compiles the code for the gradient using Automatic Differentiation (AD) with [Clad](https://github.com/vgvassilev/clad). * This analytic gradient is passed to the minimizer, which can result in significant speedups for many-parameter fits, From 7a52d59689d6f8636728efd6f375624bc1b41259 Mon Sep 17 00:00:00 2001 From: Aaron Jomy Date: Mon, 31 Mar 2025 14:27:32 +0200 Subject: [PATCH 4/4] [CPyCppyy] Drop `__array__` from std::vector pythonizations The addition of the __array__ utility to std::vector Python proxies causes a bug where the resulting array is a single dimension, causing loss of data when converting to numpy arrays, for >1dim vectors. The recursive nature of this function, passes each subarray (pydata) to the next call and only the final buffer is cast to a lowlevel view and resized (in VectorData), resulting in only the first 1D array to be returned. See https://github.com/root-project/root/issues/17729 Since this C++ pythonization was added with the upgrade in 6.32, and is only defined and used recursively, the safe option is to disable this function and no longer add it. It is temporarily removed to prevent errors due to -Wunused-function --- .../pyroot/cppyy/CPyCppyy/src/Pythonize.cxx | 16 ++++- ...std-vector-numpy-array-pythonization.patch | 65 +++++++++++++++++++ bindings/pyroot/cppyy/sync-upstream | 1 + 3 files changed, 81 insertions(+), 1 deletion(-) create mode 100644 bindings/pyroot/cppyy/patches/CPyCppyy-Disable_std-vector-numpy-array-pythonization.patch diff --git a/bindings/pyroot/cppyy/CPyCppyy/src/Pythonize.cxx b/bindings/pyroot/cppyy/CPyCppyy/src/Pythonize.cxx index 8559b2ebfe7ff..120ddb9f86f1f 100644 --- a/bindings/pyroot/cppyy/CPyCppyy/src/Pythonize.cxx +++ b/bindings/pyroot/cppyy/CPyCppyy/src/Pythonize.cxx @@ -527,6 +527,13 @@ PyObject* VectorData(PyObject* self, PyObject*) } +// This function implements __array__, added to std::vector python proxies and causes +// a bug (see explanation at Utility::AddToClass(pyclass, "__array__"...) in CPyCppyy::Pythonize) +// The recursive nature of this function, passes each subarray (pydata) to the next call and only +// the final buffer is cast to a lowlevel view and resized (in VectorData), resulting in only the +// first 1D array to be returned. See https://github.com/root-project/root/issues/17729 +// It is temporarily removed to prevent errors due to -Wunused-function, since it is no longer added. +#if 0 //--------------------------------------------------------------------------- PyObject* VectorArray(PyObject* self, PyObject* args, PyObject* kwargs) { @@ -537,7 +544,7 @@ PyObject* VectorArray(PyObject* self, PyObject* args, PyObject* kwargs) Py_DECREF(pydata); return newarr; } - +#endif //----------------------------------------------------------------------------- static PyObject* vector_iter(PyObject* v) { @@ -1810,8 +1817,15 @@ bool CPyCppyy::Pythonize(PyObject* pyclass, const std::string& name) Utility::AddToClass(pyclass, "__real_data", "data"); Utility::AddToClass(pyclass, "data", (PyCFunction)VectorData); + // The addition of the __array__ utility to std::vector Python proxies causes a + // bug where the resulting array is a single dimension, causing loss of data when + // converting to numpy arrays, for >1dim vectors. Since this C++ pythonization + // was added with the upgrade in 6.32, and is only defined and used recursively, + // the safe option is to disable this function and no longer add it. +#if 0 // numpy array conversion Utility::AddToClass(pyclass, "__array__", (PyCFunction)VectorArray, METH_VARARGS | METH_KEYWORDS /* unused */); +#endif // checked getitem if (HasAttrDirect(pyclass, PyStrings::gLen)) { diff --git a/bindings/pyroot/cppyy/patches/CPyCppyy-Disable_std-vector-numpy-array-pythonization.patch b/bindings/pyroot/cppyy/patches/CPyCppyy-Disable_std-vector-numpy-array-pythonization.patch new file mode 100644 index 0000000000000..cac7323795ba1 --- /dev/null +++ b/bindings/pyroot/cppyy/patches/CPyCppyy-Disable_std-vector-numpy-array-pythonization.patch @@ -0,0 +1,65 @@ +From 82295e09c77ae61e2fd8356be792d61addf2c801 Mon Sep 17 00:00:00 2001 +From: Aaron Jomy +Date: Mon, 31 Mar 2025 14:27:32 +0200 +Subject: [PATCH] [CPyCppyy] Drop `__array__` from std::vector pythonizations + +The addition of the __array__ utility to std::vector Python proxies causes a +bug where the resulting array is a single dimension, causing loss of data when +converting to numpy arrays, for >1dim vectors. The recursive nature of this +function, passes each subarray (pydata) to the next call and only the final +buffer is cast to a lowlevel view and resized (in VectorData), resulting in +only the first 1D array to be returned. See https://github.com/root-project/root/issues/17729 + +Since this C++ pythonization was added with the upgrade in 6.32, and is only +defined and used recursively, the safe option is to disable this function and +no longer add it. It is temporarily removed to prevent errors due to -Wunused-function +--- + bindings/pyroot/cppyy/CPyCppyy/src/Pythonize.cxx | 16 +++++++++++++++- + 1 file changed, 15 insertions(+), 1 deletion(-) + +diff --git a/bindings/pyroot/cppyy/CPyCppyy/src/Pythonize.cxx b/bindings/pyroot/cppyy/CPyCppyy/src/Pythonize.cxx +index 9b87905bab..0510c1c6ac 100644 +--- a/bindings/pyroot/cppyy/CPyCppyy/src/Pythonize.cxx ++++ b/bindings/pyroot/cppyy/CPyCppyy/src/Pythonize.cxx +@@ -527,6 +527,13 @@ PyObject* VectorData(PyObject* self, PyObject*) + } + + ++// This function implements __array__, added to std::vector python proxies and causes ++// a bug (see explanation at Utility::AddToClass(pyclass, "__array__"...) in CPyCppyy::Pythonize) ++// The recursive nature of this function, passes each subarray (pydata) to the next call and only ++// the final buffer is cast to a lowlevel view and resized (in VectorData), resulting in only the ++// first 1D array to be returned. See https://github.com/root-project/root/issues/17729 ++// It is temporarily removed to prevent errors due to -Wunused-function, since it is no longer added. ++#if 0 + //--------------------------------------------------------------------------- + PyObject* VectorArray(PyObject* self, PyObject* args, PyObject* kwargs) + { +@@ -537,7 +544,7 @@ PyObject* VectorArray(PyObject* self, PyObject* args, PyObject* kwargs) + Py_DECREF(pydata); + return newarr; + } +- ++#endif + + //----------------------------------------------------------------------------- + static PyObject* vector_iter(PyObject* v) { +@@ -1810,8 +1817,15 @@ bool CPyCppyy::Pythonize(PyObject* pyclass, const std::string& name) + Utility::AddToClass(pyclass, "__real_data", "data"); + Utility::AddToClass(pyclass, "data", (PyCFunction)VectorData); + ++ // The addition of the __array__ utility to std::vector Python proxies causes a ++ // bug where the resulting array is a single dimension, causing loss of data when ++ // converting to numpy arrays, for >1dim vectors. Since this C++ pythonization ++ // was added with the upgrade in 6.32, and is only defined and used recursively, ++ // the safe option is to disable this function and no longer add it. ++#if 0 + // numpy array conversion + Utility::AddToClass(pyclass, "__array__", (PyCFunction)VectorArray, METH_VARARGS | METH_KEYWORDS /* unused */); ++#endif + + // checked getitem + if (HasAttrDirect(pyclass, PyStrings::gLen)) { +-- +2.43.0 + diff --git a/bindings/pyroot/cppyy/sync-upstream b/bindings/pyroot/cppyy/sync-upstream index c5d5c15e36851..4b2212633a544 100755 --- a/bindings/pyroot/cppyy/sync-upstream +++ b/bindings/pyroot/cppyy/sync-upstream @@ -46,6 +46,7 @@ git apply patches/CPyCppyy-Always-convert-returned-std-string.patch git apply patches/CPyCppyy-Disable-implicit-conversion-to-smart-ptr.patch git apply patches/CPyCppyy-TString_converter.patch git apply patches/CPyCppyy-Prevent-construction-of-agg-init-for-tuple.patch +git apply patches/CPyCppyy-Disable_std-vector-numpy-array-pythonization.patch git apply patches/cppyy-No-CppyyLegacy-namespace.patch git apply patches/cppyy-Remove-Windows-workaround.patch git apply patches/cppyy-Don-t-enable-cling-autoloading.patch