ROOT 6.26 compatibility for 112x branch #776

guitargeek · 2022-07-29T12:40:29Z

This PR does the necessary changes to make the 112x branch compatible with ROOT 6.26.

Backwards compatibility is kept because there are preprocessor macros to get the right code for the right ROOT version.

ROOT 6.26 is added to the CI as suggested by @nsmith- in #772 (comment).

After #772, this contains the second part of the necessary changes that I also gave to @maxgalli to try out in his private branch:
https://github.com/maxgalli/HiggsAnalysis-CombinedLimit/commits/626-py3

gpetruc · 2022-08-19T08:41:31Z

Hi @guitargeek,
When trying to compile this in /cvmfs/sft.cern.ch/lcg/views/LCG_102/x86_64-centos8-gcc11-opt I also had two compilation errors in
gpetruc@fcfbf26 (gcc complains that this is null)
Do you happen to have also some insight about this?

guitargeek · 2022-08-19T10:34:54Z

Hi @gpetruc! I made some comments inline in your commit, but the problem is that you are dereferenicing the coef pointer, which is always nullptr because that was the condition to exit the previous loop:
while((coef=(RooAbsReal*)_coefIter->Next()))

Wow I'm amazed the compiler catches this kind of stuff now!

guitargeek · 2022-08-19T10:40:30Z

By the way, @amarini and @nsmith-, can this PR be merged?

gpetruc · 2022-08-19T11:27:18Z

Thanks @guitargeek! I had been assuming that the code had been broken in some strange way by an update in root, but you're right, the code had always been wrong and we had just never found out.
I think the proper fix is then
gpetruc@37c80f5

amarini · 2022-08-19T11:47:02Z

Hi,
this seems not compatible with 112x flow (i.e. merging into 112x and running it into 112x):

  File "/builds/cms-hcg/performances/ci/CMSSW_11_2_5/python/HiggsAnalysis/CombinedLimit/ShapeTools.py", line 645, in doCombinedDataset
    self.out._import(self.out.data_obs)
AttributeError: 'RooWorkspace' object has no attribute '_import'

Probably we will need to charge _import -> safe_import also in ShapeTools.py

(I'm also checking it standalone in root626, but iirc that worked).

guitargeek · 2022-08-19T12:03:28Z

Okay, thanks for letting me know, so ._import has to be renamed everywhere. I have updated the PR with that change.

How much do you think this renaming will be a problem? Would this entail that many combine users would have to change their code? It it would be too much trouble, we can also try to find another solution, but that would probably mean patching ROOT...

hcombbot · 2022-08-19T12:44:58Z

Pull Request Test.
Summary

Running options:

MODE : root626
COMBINE_TAG : 112x
COMBINE_REPO : cms-analysis
COMBINE_MERGE : guitargeek/112x_root-master
GITHUB_PR : 776
Ratio to reference values:

comb_2021_hbb_boosted	comb_2021_hgg	comb_2021_hinv	comb_2021_hmm	comb_2021_htt_incl	comb_2021_htt_stxs	comb_2021_hww	comb_2021_hzg	comb_2021_hzz	comb_2021_tth_hbb	comb_2021_tth_multilepton	comb_2021_vhbb
1.00	0.89	1.00	0.84	1.25	1.00	1.06	-409.93	1.00	1.00	1.00	0.98
You can find more detail at https://gitlab.cern.ch/cms-hcg/performances/ci/-/pipelines/4385870

hcombbot · 2022-08-19T13:17:26Z

Pull Request Test.
Summary

Running options:

MODE : cmssw
COMBINE_TAG : 112x
COMBINE_REPO : cms-analysis
COMBINE_MERGE : guitargeek/112x_root-master
GITHUB_PR : 776
Ratio to reference values:

comb_2021_hbb_boosted	comb_2021_hgg	comb_2021_hinv	comb_2021_hmm	comb_2021_htt_incl	comb_2021_htt_stxs	comb_2021_hww	comb_2021_hzg	comb_2021_hzz	comb_2021_tth_hbb	comb_2021_tth_multilepton	comb_2021_vhbb
1.00	1.00	1.00	1.00	1.00	1.00	1.00	0.94	1.00	1.00	1.00	1.00
You can find more detail at https://gitlab.cern.ch/cms-hcg/performances/ci/-/pipelines/4385869

hcombbot · 2022-08-19T13:20:56Z

Pull Request Test.
Summary

Running options:

MODE : cmssw
COMBINE_TAG : 112x
COMBINE_REPO : cms-analysis
COMBINE_MERGE : guitargeek/112x_root-master
GITHUB_PR : 776
Ratio to reference values:

comb_2019_hbb_boosted_standalone	comb_2019_hgg	comb_2019_hmm	comb_2019_htt	comb_2019_hww	comb_2019_tth_hbb	comb_2019_tth_hgg	comb_2019_tth_multilepton	comb_2019_vh_htt	comb_2019_vhbb	comb_2019_vhbb2017
1.00	0.96	0.99	1.00	1.00	1.00	0.67	1.00	1.01	1.00	1.00
You can find more detail at https://gitlab.cern.ch/cms-hcg/performances/ci/-/pipelines/4385861

amarini · 2022-08-19T13:21:52Z

I think that the change

_import -> safe_import

In the models developed privately is a minor change to the code. The important thing is that more or less the datacards continue to work or we understand why they do not.

At the moment in root626:

hzz has the error, but it seems to converge to the right value:

[#0] WARNING:Eval -- Evaluating RooAddPdf without a defined normalization set. This can lead to ambiguos coefficients definition and incorrect results. Use RooAddPdf::fixCoefNormalization(nset) to provide a normalization set for defining uniquely RooAddPdf coefficients!

while all the datacards with discrete parameters fail the fit. (hgg, hmm, hzg, ...)
htt_incl, vhbb, I don't know why it fails.

In 112x seems to be fine.

I think we can merge it, but I would say that we have not fully commissioned root626, yet.

guitargeek · 2022-08-19T14:44:19Z

Okay, thanks! Yes this RooAddPdf warning is a new warning that we print out. But depending on the context it might be harmless, for example if getVal() was only called without the normalization set to print the PDF or something. Still, it's probably something we should look at when commissioning 6.26

Also nameSet2ByIndex with selectFromSet2. This follows the interface change done in ROOT PR root-project/root#7819.

This is to adapt to the changes that were made for ROOT 6.26, where the raw pointers in ToyMCSampler were changed to `std::unique_ptr`. root-project/root#7904

Explicitely set defaultMinimizerAlgo in CascadeMinimizer to `Migrad`. As of ROOT 6.26, the `ROOT::Math::MinimizerOptions::DefaultMinimizerAlgo()` returns an empty string instead of "Migrad", like it did in ROOT 6.24. This caused errors because RooFit needs to be told what is the minimizer algo. A simple fix for this problem is to make the default minimizer algo explicit, which is probably better anyway.

After the following ROOT PR, it is not possible anymore to set the `_import` attribute of a RooWorkspace instance as of ROOT 6.26: root-project/root#9896

guitargeek · 2022-08-20T13:46:50Z

I just force-pushed to fix a typo I made in the comments

hcombbot · 2022-08-20T15:14:21Z

Pull Request Test.
Summary

Running options:

MODE : cmssw
COMBINE_TAG : 112x
COMBINE_REPO : cms-analysis
COMBINE_MERGE : guitargeek/112x_root-master
GITHUB_PR : 776
Ratio to reference values:

comb_2019_hbb_boosted_standalone	comb_2019_hgg	comb_2019_hmm	comb_2019_htt	comb_2019_hww	comb_2019_tth_hbb	comb_2019_tth_hgg	comb_2019_tth_multilepton	comb_2019_vh_htt	comb_2019_vhbb	comb_2019_vhbb2017
1.00	0.96	0.99	1.30	1.00	1.00	0.67	1.00	1.01	1.00	1.00
You can find more detail at https://gitlab.cern.ch/cms-hcg/performances/ci/-/pipelines/4389631

nsmith- · 2022-08-22T15:13:18Z

Do you want to cherry-pick gpetruc@37c80f5 ? I guess the compilation succeeds in the github CI because it is getting gcc v10.4, and LCG102 has gcc v11.2

guitargeek · 2022-08-23T11:08:52Z

Okay, I have done that!

hcombbot · 2022-08-23T12:27:44Z

Pull Request Test.
Summary

Running options:

MODE : cmssw
COMBINE_TAG : 112x
COMBINE_REPO : cms-analysis
COMBINE_MERGE : guitargeek/112x_root-master
GITHUB_PR : 776
Ratio to reference values:

comb_2019_hbb_boosted_standalone	comb_2019_hgg	comb_2019_hmm	comb_2019_htt	comb_2019_hww	comb_2019_tth_hbb	comb_2019_tth_hgg	comb_2019_tth_multilepton	comb_2019_vh_htt	comb_2019_vhbb	comb_2019_vhbb2017
1.00	0.96	0.99	1.30	1.00	1.00	0.67	1.00	1.01	1.00	1.00
You can find more detail at https://gitlab.cern.ch/cms-hcg/performances/ci/-/pipelines/4400168

hcombbot · 2022-08-23T14:00:11Z

Pull Request Test.
Summary

Running options:

MODE : cmssw
COMBINE_TAG : 112x
COMBINE_REPO : cms-analysis
COMBINE_MERGE : guitargeek/112x_root-master
GITHUB_PR : 776
Ratio to reference values:

comb_2019_hbb_boosted_standalone	comb_2019_hgg	comb_2019_hmm	comb_2019_htt	comb_2019_hww	comb_2019_tth_hbb	comb_2019_tth_hgg	comb_2019_tth_multilepton	comb_2019_vh_htt	comb_2019_vhbb	comb_2019_vhbb2017
1.00	1.01	0.70	1.00	1.00	1.00	1.01	1.00	1.00	1.00	1.00
You can find more detail at https://gitlab.cern.ch/cms-hcg/performances/ci/-/pipelines/4400765

amarini · 2022-08-23T14:07:45Z

I merge this. We will investigate the discrepancies in ROOT626 in the future. The last run I only changed some minimizer options.

amarini added the safe to test label Jul 29, 2022

guitargeek force-pushed the 112x_root-master branch 4 times, most recently from 60ad240 to 70dad33 Compare July 29, 2022 18:20

guitargeek force-pushed the 112x_root-master branch from 70dad33 to 40ee895 Compare August 19, 2022 12:00

guitargeek added 5 commits August 20, 2022 15:45

Replace nameSet1ByIndex with selectFromSet1 in ROOT >= 6.26

c8892f7

Also nameSet2ByIndex with selectFromSet2. This follows the interface change done in ROOT PR root-project/root#7819.

Adapt ToyMCSamplerOpt to change in ToyMCSampler with ROOT 6.26

4f299d0

This is to adapt to the changes that were made for ROOT 6.26, where the raw pointers in ToyMCSampler were changed to `std::unique_ptr`. root-project/root#7904

Don't try to reset _import attribute of RooWorkspace instance

5f707c8

After the following ROOT PR, it is not possible anymore to set the `_import` attribute of a RooWorkspace instance as of ROOT 6.26: root-project/root#9896

Add ROOT 6.26 to the CI

7b55b80

guitargeek force-pushed the 112x_root-master branch from 40ee895 to 7b55b80 Compare August 20, 2022 13:45

nsmith- mentioned this pull request Aug 22, 2022

Excessive allocations in calculatePartialBinVolume slowing down fits #786

Open

Fix compilation in LCG_102 stack

b6252af

amarini added the port to 112x-comb2022 label Aug 23, 2022

amarini merged commit f085fb7 into cms-analysis:112x Aug 23, 2022

guitargeek deleted the 112x_root-master branch August 23, 2022 14:36

guitargeek restored the 112x_root-master branch August 23, 2022 14:36

guitargeek deleted the 112x_root-master branch August 23, 2022 15:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ROOT 6.26 compatibility for 112x branch #776

ROOT 6.26 compatibility for 112x branch #776

guitargeek commented Jul 29, 2022 •

edited

gpetruc commented Aug 19, 2022

guitargeek commented Aug 19, 2022 •

edited

guitargeek commented Aug 19, 2022

gpetruc commented Aug 19, 2022

amarini commented Aug 19, 2022

guitargeek commented Aug 19, 2022

hcombbot commented Aug 19, 2022

hcombbot commented Aug 19, 2022

hcombbot commented Aug 19, 2022

amarini commented Aug 19, 2022

guitargeek commented Aug 19, 2022

guitargeek commented Aug 20, 2022

hcombbot commented Aug 20, 2022

nsmith- commented Aug 22, 2022

guitargeek commented Aug 23, 2022

hcombbot commented Aug 23, 2022

hcombbot commented Aug 23, 2022

amarini commented Aug 23, 2022

ROOT 6.26 compatibility for 112x branch #776

ROOT 6.26 compatibility for 112x branch #776

Conversation

guitargeek commented Jul 29, 2022 • edited

gpetruc commented Aug 19, 2022

guitargeek commented Aug 19, 2022 • edited

guitargeek commented Aug 19, 2022

gpetruc commented Aug 19, 2022

amarini commented Aug 19, 2022

guitargeek commented Aug 19, 2022

hcombbot commented Aug 19, 2022

Pull Request Test. Summary

hcombbot commented Aug 19, 2022

Pull Request Test. Summary

hcombbot commented Aug 19, 2022

Pull Request Test. Summary

amarini commented Aug 19, 2022

guitargeek commented Aug 19, 2022

guitargeek commented Aug 20, 2022

hcombbot commented Aug 20, 2022

Pull Request Test. Summary

nsmith- commented Aug 22, 2022

guitargeek commented Aug 23, 2022

hcombbot commented Aug 23, 2022

Pull Request Test. Summary

hcombbot commented Aug 23, 2022

Pull Request Test. Summary

amarini commented Aug 23, 2022

guitargeek commented Jul 29, 2022 •

edited

guitargeek commented Aug 19, 2022 •

edited

Pull Request Test.
Summary

Pull Request Test.
Summary

Pull Request Test.
Summary

Pull Request Test.
Summary

Pull Request Test.
Summary

Pull Request Test.
Summary