[HS3][RF][HistFactory] improvements for OverallSys and constraint handling, general code cleanup #18135

cburgard · 2025-03-25T16:07:37Z

This Pull request:

Achieves full round-trip compatibility with the latest round of ATLAS public likelihoods, encoding results presented at Moriond EW and MoriondQCD 2025.

Changes or fixes:

factorize the HistFactory export into several functions for easier maintenance
make HistFactory FlexibleInterpVar actually respect non-standard (!=4) interpolation codes when roundtripping to HS3
allow smartly decomposing RooFormulaVars that appear as elements of any product inside a HistFactory Sample, but that are themselves just products, into their components (including recursion)
allow smarty decomposing RooFormulaVars that appear as elements of any product inside the HistFactory sample (see above), but that are themselves just attempts at encoding a linear FlexibleInterpVar behavior (aka 1+c*x) and encoding them as actual entries in the FlexibleInterpVar of that sample
detect constraint terms that relate to a sample, but are not part of any standard modifier type to be exported as JSON, and emit a warning to notify the user of their existence (until we have made up our mind on how to handle globalObservables in HS3)

Checklist:

tested changes locally
updated the docs (if necessary)

This PR fixes #

github-actions · 2025-03-25T18:43:30Z

Test Results

19 files 19 suites 4d 17h 24m 5s ⏱️
2 722 tests 2 721 ✅ 0 💤 1 ❌
49 963 runs 49 962 ✅ 0 💤 1 ❌

For more details on these failures, see this check.

Results for commit a080f42.

♻️ This comment has been updated with latest results.

Various changes, among which are (1) respecting interpolation codes, (2) automatically parsing fancy RooFormulaStrings that are just attempts at replicating a FlexibleInterpVar, and (3) doing the same for RooFormulaStrings that are just products, or combinations thereof.

These classes are very small, it's not necessary to split things up in different files at that granularity.

Duplicating the interface for the function state is not necessary, verbose and prone to copy-paste errors, and most users use the MnApplication via the `ROOT::Minimizer` anyway.

guitargeek

Thanks! I also added come code improvement commits to make sure we're not adding nt number of lines to ROOT, which I think is good practice 🙂

smuzaffar · 2025-06-27T14:23:40Z

while test latest root changes in cmssw, we are getting the following build time error . @guitargeek any suggestions how to avoid/fix https://github.com/cms-sw/cmssw/blob/master/RecoVertex/BeamSpotProducer/src/PVFitter.cc#L241 ?

/data/cmsbld/jenkins/workspace/ib-run-pr-tests/testBuildDir/el8_amd64_gcc12/external/gcc/12.3.1-40d504be6370b5a30e3947a6e575ca28/bin/c++ -c -DCMS_MICRO_ARCH='x86-64-v3' -DGNU_GCC -D_GNU_SOURCE -DTBB_USE_GLIBCXX_VERSION=120301 -DTBB_SUPPRESS_DEPRECATED_MESSAGES -DTBB_PREVIEW_RESUMABLE_TASKS=1 -DTBB_PREVIEW_TASK_GROUP_EXTENSIONS=1 -DBOOST_SPIRIT_THREADSAFE -DPHOENIX_THREADSAFE -DBOOST_MATH_DISABLE_STD_FPCLASSIFY -DBOOST_UUID_RANDOM_PROVIDER_FORCE_POSIX -DBOOST_MPL_IGNORE_PARENTHESES_WARNING -DCMSSW_GIT_HASH='CMSSW_15_1_ROOT6_X_2025-06-26-2300' -DPROJECT_NAME='CMSSW' -DPROJECT_VERSION='CMSSW_15_1_ROOT6_X_2025-06-26-2300' -Isrc -Ipoison -I/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02895/el8_amd64_gcc12/cms/cmssw-patch/CMSSW_15_1_ROOT6_X_2025-06-26-2300/src -I/data/cmsbld/jenkins/workspace/ib-run-pr-tests/testBuildDir/el8_amd64_gcc12/cms/coral/CORAL_2_3_21-9a3f73b9647d58b905d7d1c5689b8643/include/LCG -I/data/cmsbld/jenkins/workspace/ib-run-pr-tests/testBuildDir/el8_amd64_gcc12/external/alpaka/1.2.0-23a2bf2e896b7aace8e772f289604b47/include -I/data/cmsbld/jenkins/workspace/ib-run-pr-tests/testBuildDir/el8_amd64_gcc12/external/pcre/8.43-2d141998cfe5424b8f7aff48035cc2da/include -isystem/data/cmsbld/jenkins/workspace/ib-run-pr-tests/testBuildDir/el8_amd64_gcc12/external/boost/1.80.0-189b192d618e9605b04b60048d1376aa/include -I/data/cmsbld/jenkins/workspace/ib-run-pr-tests/testBuildDir/el8_amd64_gcc12/external/bz2lib/1.0.6-d065ccd79984efc6d4660f410e4c81de/include -isystem/data/cmsbld/jenkins/workspace/ib-run-pr-tests/testBuildDir/el8_amd64_gcc12/external/clhep/2.4.7.1-d3a3e353d370e701238f7949a0d7909f/include -I/data/cmsbld/jenkins/workspace/ib-run-pr-tests/testBuildDir/el8_amd64_gcc12/external/curl/7.79.0-d8c0d5017cfad573d276c56bd08114b8/include -I/data/cmsbld/jenkins/workspace/ib-run-pr-tests/testBuildDir/el8_amd64_gcc12/external/gsl/2.6-f7574c606b0ce57ff601d3ca9534cd01/include -I/data/cmsbld/jenkins/workspace/ib-run-pr-tests/testBuildDir/el8_amd64_gcc12/external/libuuid/2.34-27ce4c3579b5b1de2808ea9c4cd8ed29/include -isystem/data/cmsbld/jenkins/workspace/ib-run-pr-tests/testBuildDir/el8_amd64_gcc12/lcg/root/6.37.1-f9aee8a75049a798245cd591a4a5bdb0/include -isystem/data/cmsbld/jenkins/workspace/ib-run-pr-tests/testBuildDir/el8_amd64_gcc12/external/tbb/v2022.0.0-79b5a917b0c13f831cd534a5b9f53a95/include -I/data/cmsbld/jenkins/workspace/ib-run-pr-tests/testBuildDir/el8_amd64_gcc12/external/xerces-c/3.1.3-c7b88eaa36d0408120f3c29826a04bf6/include -I/data/cmsbld/jenkins/workspace/ib-run-pr-tests/testBuildDir/el8_amd64_gcc12/external/xz/5.2.5-6f3f49b07db84e10c9be594a1176c114/include -I/data/cmsbld/jenkins/workspace/ib-run-pr-tests/testBuildDir/el8_amd64_gcc12/external/zlib/1.2.13-d217cdbdd8d586e845e05946de2796be/include -I/data/cmsbld/jenkins/workspace/ib-run-pr-tests/testBuildDir/el8_amd64_gcc12/external/eigen/3bb6a48d8c171cf20b5f8e48bfb4e424fbd4f79e-5d91c922e771c0dc4f6bc00f61f3e2c5/include -I/data/cmsbld/jenkins/workspace/ib-run-pr-tests/testBuildDir/el8_amd64_gcc12/external/eigen/3bb6a48d8c171cf20b5f8e48bfb4e424fbd4f79e-5d91c922e771c0dc4f6bc00f61f3e2c5/include/eigen3 -I/data/cmsbld/jenkins/workspace/ib-run-pr-tests/testBuildDir/el8_amd64_gcc12/external/fmt/10.2.1-e35fd1db5eb3abc8ac0452e8ee427196/include -I/data/cmsbld/jenkins/workspace/ib-run-pr-tests/testBuildDir/el8_amd64_gcc12/external/md5/1.0.0-5b594b264e04ae51e893b1d69a797ec6/include -I/data/cmsbld/jenkins/workspace/ib-run-pr-tests/testBuildDir/el8_amd64_gcc12/external/OpenBLAS/0.3.27-70a9dd2c9f309171934f13e3003b0540/include -I/data/cmsbld/jenkins/workspace/ib-run-pr-tests/testBuildDir/el8_amd64_gcc12/external/tinyxml2/6.2.0-a0ad3950415fa3138d99b7da42eb4c9f/include -O3 -pthread -pipe -Werror=main -Werror=pointer-arith -Werror=overlength-strings -Wno-vla -Werror=overflow -std=c++20 -ftree-vectorize -Werror=array-bounds -Werror=format-contains-nul -Werror=type-limits -fvisibility-inlines-hidden -fno-math-errno --param vect-max-version-for-alias-checks=50 -Xassembler --compress-debug-sections -Wno-error=array-bounds -Warray-bounds -fuse-ld=bfd -march=x86-64-v3 -felide-constructors -fmessage-length=0 -Wall -Wno-non-template-friend -Wno-long-long -Wreturn-type -Wextra -Wpessimizing-move -Wclass-memaccess -Wno-cast-function-type -Wno-unused-but-set-parameter -Wno-ignored-qualifiers -Wno-unused-parameter -Wunused -Wparentheses -Werror=return-type -Werror=missing-braces -Werror=unused-value -Werror=unused-label -Werror=address -Werror=format -Werror=sign-compare -Werror=write-strings -Werror=delete-non-virtual-dtor -Werror=strict-aliasing -Werror=narrowing -Werror=unused-but-set-variable -Werror=reorder -Werror=unused-variable -Werror=conversion-null -Werror=return-local-addr -Wnon-virtual-dtor -Werror=switch -fdiagnostics-show-option -Wno-unused-local-typedefs -Wno-attributes -Wno-psabi -DEIGEN_DONT_PARALLELIZE -DEIGEN_MAX_ALIGN_BYTES=64 -Wno-error=unused-variable -DALPAKA_DEFAULT_HOST_MEMORY_ALIGNMENT=128 -DALPAKA_DISABLE_VENDOR_RNG -DBOOST_DISABLE_ASSERTS -flto=auto -fipa-icf -flto-odr-type-merging -fno-fat-lto-objects -Wodr -fPIC -MMD -MF tmp/el8_amd64_gcc12/src/RecoVertex/BeamSpotProducer/src/RecoVertexBeamSpotProducer/SealModules.cc.d src/RecoVertex/BeamSpotProducer/src/SealModules.cc -o tmp/el8_amd64_gcc12/src/RecoVertex/BeamSpotProducer/src/RecoVertexBeamSpotProducer/SealModules.cc.o
src/RecoVertex/BeamSpotProducer/src/PVFitter.cc: In member function 'bool PVFitter::runFitter()':
  [src/RecoVertex/BeamSpotProducer/src/PVFitter.cc:410](https://github.com/cms-sw/cmssw/blob/CMSSW_15_1_ROOT6_X_2025-06-26-2300/RecoVertex/BeamSpotProducer/src/PVFitter.cc#L410):12: error: 'class ROOT::Minuit2::MnMigrad' has no member named 'Fix'
   410 |     migrad.Fix(4);
      |            ^~~
  [src/RecoVertex/BeamSpotProducer/src/PVFitter.cc:411](https://github.com/cms-sw/cmssw/blob/CMSSW_15_1_ROOT6_X_2025-06-26-2300/RecoVertex/BeamSpotProducer/src/PVFitter.cc#L411):12: error: 'class ROOT::Minuit2::MnMigrad' has no member named 'Fix'
   411 |     migrad.Fix(6);
      |            ^~~
  [src/RecoVertex/BeamSpotProducer/src/PVFitter.cc:412](https://github.com/cms-sw/cmssw/blob/CMSSW_15_1_ROOT6_X_2025-06-26-2300/RecoVertex/BeamSpotProducer/src/PVFitter.cc#L412):12: error: 'class ROOT::Minuit2::MnMigrad' has no member named 'Fix'
   412 |     migrad.Fix(7);
      |            ^~~
  [src/RecoVertex/BeamSpotProducer/src/PVFitter.cc:413](https://github.com/cms-sw/cmssw/blob/CMSSW_15_1_ROOT6_X_2025-06-26-2300/RecoVertex/BeamSpotProducer/src/PVFitter.cc#L413):12: error: 'class ROOT::Minuit2::MnMigrad' has no member named 'Fix'
   413 |     migrad.Fix(9);
      |            ^~~
  [src/RecoVertex/BeamSpotProducer/src/PVFitter.cc:442](https://github.com/cms-sw/cmssw/blob/CMSSW_15_1_ROOT6_X_2025-06-26-2300/RecoVertex/BeamSpotProducer/src/PVFitter.cc#L442):12: error: 'class ROOT::Minuit2::MnMigrad' has no member named 'Release'
   442 |     migrad.Release(4);
      |            ^~~~~~~
  [src/RecoVertex/BeamSpotProducer/src/PVFitter.cc:443](https://github.com/cms-sw/cmssw/blob/CMSSW_15_1_ROOT6_X_2025-06-26-2300/RecoVertex/BeamSpotProducer/src/PVFitter.cc#L443):12: error: 'class ROOT::Minuit2::MnMigrad' has no member named 'Release'
   443 |     migrad.Release(6);
      |            ^~~~~~~
  [src/RecoVertex/BeamSpotProducer/src/PVFitter.cc:444](https://github.com/cms-sw/cmssw/blob/CMSSW_15_1_ROOT6_X_2025-06-26-2300/RecoVertex/BeamSpotProducer/src/PVFitter.cc#L444):12: error: 'class ROOT::Minuit2::MnMigrad' has no member named 'Release'
   444 |     migrad.Release(7);

guitargeek · 2025-06-27T15:16:43Z

Hi @smuzaffar, this PR should fix it: cms-sw/cmssw#48433

smuzaffar · 2025-06-27T15:26:34Z

thanks a lot @guitargeek for cmssw PR

cburgard requested review from lmoneta and guitargeek as code owners March 25, 2025 16:07

cburgard changed the title ~~[HS3][RF][HistFactory]~~ [HS3][RF][HistFactory] improvements for OverallSys and constraint handling, general code cleanup Mar 25, 2025

devajithvs assigned lmoneta and guitargeek Apr 3, 2025

cburgard force-pushed the hs3-histfactory-cleanup branch from 428cf27 to a080f42 Compare April 9, 2025 08:49

cburgard force-pushed the hs3-histfactory-cleanup branch from a080f42 to c18a3bb Compare May 13, 2025 12:56

cburgard added 4 commits June 26, 2025 18:25

[RF][HS3] Refactored HistFactory export into several functions

de9e4b6

[RF][HS3] Refactoring to be able to identify missing constraints terms

1751b42

[RF][HS3] Add option to delete temporary objects once export is finished

0824b5e

guitargeek force-pushed the hs3-histfactory-cleanup branch from e4f5f54 to f2b6adf Compare June 26, 2025 17:28

guitargeek requested a review from bellenot as a code owner June 26, 2025 17:28

cburgard and others added 6 commits June 26, 2025 20:02

[RF][HS3] More explicit constraint name handling

3c9f226

[RF][HS3] Important bugfix to extraction of hand-coded formulas

5dd7303

[RF] Some refactoring to avoid code duplication

56b7774

[Minuit2] Merge parameter transformation classes into one file

4afd58d

These classes are very small, it's not necessary to split things up in different files at that granularity.

[Math] Remove no-op function Fitter::ExamineFCN

246fed2

[Minuit2] Remove facade interface of MnApplication

f0fa2de

Duplicating the interface for the function state is not necessary, verbose and prone to copy-paste errors, and most users use the MnApplication via the `ROOT::Minimizer` anyway.

guitargeek force-pushed the hs3-histfactory-cleanup branch from f2b6adf to f0fa2de Compare June 26, 2025 18:05

guitargeek approved these changes Jun 26, 2025

View reviewed changes

guitargeek merged commit e8a2fba into root-project:master Jun 26, 2025
23 of 24 checks passed

guitargeek deleted the hs3-histfactory-cleanup branch June 26, 2025 22:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[HS3][RF][HistFactory] improvements for OverallSys and constraint handling, general code cleanup #18135

[HS3][RF][HistFactory] improvements for OverallSys and constraint handling, general code cleanup #18135

Uh oh!

cburgard commented Mar 25, 2025

Uh oh!

github-actions bot commented Mar 25, 2025 •

edited

Loading

Uh oh!

guitargeek left a comment

Uh oh!

Uh oh!

smuzaffar commented Jun 27, 2025

Uh oh!

guitargeek commented Jun 27, 2025

Uh oh!

smuzaffar commented Jun 27, 2025

Uh oh!

Uh oh!

[HS3][RF][HistFactory] improvements for OverallSys and constraint handling, general code cleanup #18135

[HS3][RF][HistFactory] improvements for OverallSys and constraint handling, general code cleanup #18135

Uh oh!

Conversation

cburgard commented Mar 25, 2025

This Pull request:

Changes or fixes:

Checklist:

Uh oh!

github-actions bot commented Mar 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

guitargeek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

smuzaffar commented Jun 27, 2025

Uh oh!

guitargeek commented Jun 27, 2025

Uh oh!

smuzaffar commented Jun 27, 2025

Uh oh!

Uh oh!

github-actions bot commented Mar 25, 2025 •

edited

Loading