convert MVA models for electronMVAValueMapProducer from XML to ROOT

[Dr15Jones]

Similar to #34707, the profile report for workflow 11834.21 for step5 (generating the NANO AOD) has its startup time dominated by parsing the XML files used by ElectronMVAValueMapProducer.

https://cmssdt.cern.ch/SDT/cgi-bin/igprof-navigator/CMSSW_12_4_X_2022-05-08-2300/slc7_amd64_gcc10/pp/11834.21_TTbar_14TeV+2021PU_ProdLike+TTbar_14TeV_TuneCP5_GenSim+DigiPU+RecoNanoPU+MiniAODPU+NanoPU/step5___b6ff2d761a23670d9b27095006d0e928___10_EndOfJob/37

As a test, I used convertXMLToGBRForestROOT to transform one of the XML files to it's root equivalent, the size difference was nearly a factor of 10 smaller than the gzipped XML.

-rw-r--r--. 1 chrjones zh 1059817 May  9 17:16 EIDmva_EB1_10_2017_puinfo_BDT.weights.xml.gz
-rw-r--r--. 1 chrjones zh  124872 May  9 17:17 EIDmva_EB1_10_2017_puinfo_BDT.weights.root

I would expect performance to be dramatically better as well (given similar conversions done before).

[Dr15Jones]

assign reconstruction

[cmsbuild]

New categories assigned: reconstruction

@jpata,@slava77,@clacaputo you have been requested to review this Pull request/Issue and eventually sign? Thanks

[cmsbuild]

A new Issue was created by @Dr15Jones Chris Jones.

@Dr15Jones, @perrotta, @dpiparo, @makortel, @smuzaffar, @qliphy can you please review it and eventually sign/assign? Thanks.

cms-bot commands are listed here

[jpata]

@wrtabb @SohamBhattacharya @cms-sw/egamma-pog-l2 could you please take a look at this?

[jpata]

type egamma

[jpata]

type performance-improvements

[swagata87]

@wrtabb @SohamBhattacharya @cms-sw/egamma-pog-l2 could you please take a look at this?

I had a quick look,
when the support for root was provided (xml was always supported), the support was only for loading the weights, as I see here

cmssw/CommonTools/MVAUtils/src/GBRForestTools.cc

Lines 123 to 131 in 3c60d77

	//
	// Load weights file, for ROOT file
	//
	if (reco::details::hasEnding(weightsFileFullPath, ".root")) {
	TFile gbrForestFile(weightsFileFullPath.c_str());
	std::unique_ptr<GBRForest> up(gbrForestFile.Get<GBRForest>("gbrForest"));
	gbrForestFile.Close("nodelete");
	return up;
	}

It seems we also need support for adding variables, like it is done here for xml:

cmssw/CommonTools/MVAUtils/src/GBRForestTools.cc

Lines 148 to 149 in 3c60d77

	tinyxml2::XMLElement* root = xmlDoc.FirstChildElement("MethodSetup");
	readVariables(root->FirstChildElement("Variables"), "Variable", varNames);

is there a way to support doing the above thing in root?

If this can be done, then the rest should be easy, if I understood correctly

[swagata87]

@guitargeek, since I see that you are still doing PRs for cmssw, may be this one could be of your interest.
You don't have to do it, and there is no rush, but if you decide to give it a shot, you're most welcome :-D

I tried the easy thing first, just changed the xmls to root by using convertXMLToGBRForestROOT as mentioned in the issue description[1], and changed the names of files accordingly[2]. But it crashes like this, so clearly there is more to it, and it looks to me that a change in GBRForestTools.cc might be needed. But you'd know it better than I do ;-)

[1]https://github.com/cms-data/RecoEgamma-ElectronIdentification/compare/master...swagata87:xmltoroot?expand=1
[2]https://github.com/cms-sw/cmssw/compare/master...swagata87:mvaV1?expand=1

[jpata]

+reconstruction

• addressed in [EGM MVA ID] xml.gz to root conversion  #38339, Convert xml.gz files to .root files cms-data/RecoEgamma-ElectronIdentification#26, at least for most of the models

[cmsbuild]

This issue is fully signed and ready to be closed.