PFCandidate no longer inherits from CompositeCandidate #38999

Dr15Jones · 2022-08-08T17:41:48Z

PR description:

The storing of PFCandidates takes 1/3rd of the entire time it takes to write AOD. I have started to look at the class to see if there was a way to make it easier for ROOT to store. The first think I tried to was to remove CompositeCandidate (as that looks hard to store) in order to see what would break and therefore would have to be changed if I removed the inheritance. To my surprise no code in CMSSW made use of the possible compositeness of the class.

PR validation:

The Code and all dependent code compiles.

No code in CMSSW made use of the possible compositeness of the class.

cmsbuild · 2022-08-08T17:50:10Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38999/31468

This PR adds an extra 32KB to repository

cmsbuild · 2022-08-08T17:50:33Z

A new Pull Request was created by @Dr15Jones (Chris Jones) for master.

It involves the following packages:

DataFormats/ParticleFlowCandidate (reconstruction)
DataFormats/PatCandidates (reconstruction)

@jpata, @cmsbuild, @clacaputo can you please review it and eventually sign? Thanks.
@gouskos, @rovere, @lgray, @missirol, @hatakeyamak, @gpetruc this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy, @rappoccio you are the release manager for this.

cms-bot commands are listed here

Dr15Jones · 2022-08-08T18:00:56Z

please test

Dr15Jones · 2022-08-08T21:06:43Z

I did a test where I wrote a 10,000 event file containing only recoPFCandidates_particleFlow__RECO using standard AOD settings. If I wrote the file with no compression, this PR made the output module run 40% faster. If I used the standard AOD compression, the times were the same within measurement error (just 1% difference).

cmsbuild · 2022-08-08T21:57:42Z

-1

Failed Tests: UnitTests RelVals RelVals-INPUT AddOn
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-d5d55e/26710/summary.html
COMMIT: 85e0542
CMSSW: CMSSW_12_5_X_2022-08-08-1100/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/38999/26710/install.sh to create a dev area with all the needed externals and cmssw changes.

Unit Tests

I found errors in the following unit tests:

---> test runtestTqafTopJetCombination had ERRORS
---> test runtestTqafTopEventSelection had ERRORS
---> test runtestTqafTopTools had ERRORS
---> test runtestTqafTopHitFit had ERRORS
and more ...

RelVals

136.8311136.8311_RunJetHT2017F_reminiaod+RunJetHT2017F_reminiaod+REMINIAOD_data2017+HARVEST2017_REMINIAOD_data2017/step2_RunJetHT2017F_reminiaod+RunJetHT2017F_reminiaod+REMINIAOD_data2017+HARVEST2017_REMINIAOD_data2017.log
136.7611136.7611_RunJetHT2016E_reminiaod+RunJetHT2016E_reminiaod+REMINIAOD_data2016_HIPM+HARVESTDR2_REMINIAOD_data2016_HIPM/step2_RunJetHT2016E_reminiaod+RunJetHT2016E_reminiaod+REMINIAOD_data2016_HIPM+HARVESTDR2_REMINIAOD_data2016_HIPM.log
136.88811136.88811_RunJetHT2018D_reminiaodUL+RunJetHT2018D_reminiaodUL+REMINIAOD_data2018UL+HARVEST2018_REMINIAOD_data2018UL/step2_RunJetHT2018D_reminiaodUL+RunJetHT2018D_reminiaodUL+REMINIAOD_data2018UL+HARVEST2018_REMINIAOD_data2018UL.log

Expand to see more relval errors ...

158.01

RelVals-INPUT

136.72411136.72411_RunJetHT2016B_reminiaodUL+RunJetHT2016B_reminiaodUL+REMINIAOD_data2016UL_HIPM+HARVESTDR2_REMINIAOD_data2016UL_HIPM/step2_RunJetHT2016B_reminiaodUL+RunJetHT2016B_reminiaodUL+REMINIAOD_data2016UL_HIPM+HARVESTDR2_REMINIAOD_data2016UL_HIPM.log
136.72412136.72412_RunJetHT2016B_reminiaodUL+RunJetHT2016B_reminiaodUL+REMININANO_data2016UL_HIPM+HARVESTDR2_REMININANO_data2016UL_HIPM/step2_RunJetHT2016B_reminiaodUL+RunJetHT2016B_reminiaodUL+REMININANO_data2016UL_HIPM+HARVESTDR2_REMININANO_data2016UL_HIPM.log
136.7611136.7611_RunJetHT2016E_reminiaod+RunJetHT2016E_reminiaod+REMINIAOD_data2016_HIPM+HARVESTDR2_REMINIAOD_data2016_HIPM/step2_RunJetHT2016E_reminiaod+RunJetHT2016E_reminiaod+REMINIAOD_data2016_HIPM+HARVESTDR2_REMINIAOD_data2016_HIPM.log

Expand to see more relval errors ...

AddOn Tests

pat1cmsRun /data/cmsbld/jenkins/workspace/ib-run-pr-addon/CMSSW_12_5_X_2022-08-08-1100/src/PhysicsTools/PatAlgos/test/IntegrationTest_cfg.py : FAILED - time: date Mon Aug 8 21:01:15 2022-date Mon Aug 8 20:59:52 2022 s - exit: 35584

Dr15Jones · 2022-08-09T14:09:40Z

@pcanal So I changed the inheritance for reco::PFCandidate, dropping one of the intermediate base classes. When I did that I updated all the class versions as well. However, now the tests run for the PR are failing where all these tests seem to be reading back reco::PFCandidates which had been stored using the previous version. It seems like the scheme evolution isn't working properly.

Dr15Jones · 2022-08-09T16:03:43Z

So I wrote a simple EDAnalyzer which dumps the contents of a std::vector<reco::PFCandidate>. I ran it using both the original code and the new code, both reading the same input file (which was written using the original format). The results are

element 0	PFCandidate type: 1 E/pT/eta/phi 0.811/0.175/-2.196/-0.403, blocks/ie |	element 0	PFCandidate type: 1 E/pT/eta/phi 0.000/0.175/-2.196/-0.403, blocks/ie
element 1	PFCandidate type: 1 E/pT/eta/phi 1.500/0.230/2.559/2.968, blocks/iele |	element 1	PFCandidate type: 1 E/pT/eta/phi 0.000/0.230/2.559/2.968, blocks/iele
element 2	PFCandidate type: 1 E/pT/eta/phi 0.589/0.180/1.825/-2.992, blocks/iel |	element 2	PFCandidate type: 1 E/pT/eta/phi 0.000/0.180/1.825/-2.992, blocks/iel
element 3	PFCandidate type: 1 E/pT/eta/phi 1.244/0.205/2.484/-3.059, blocks/iel |	element 3	PFCandidate type: 1 E/pT/eta/phi 0.000/0.205/2.484/-3.059, blocks/iel
element 4	PFCandidate type: 1 E/pT/eta/phi 9.328/2.029/-2.206/-1.708, blocks/ie |	element 4	PFCandidate type: 1 E/pT/eta/phi 0.000/2.029/-2.206/-1.708, blocks/ie
element 5	PFCandidate type: 1 E/pT/eta/phi 2.963/0.630/-2.229/-0.503, blocks/ie |	element 5	PFCandidate type: 1 E/pT/eta/phi 0.000/0.630/-2.229/-0.503, blocks/ie

So when reading back using the new format, energy is always 0 while pT, eta, and phi look fine.

pcanal · 2022-08-09T16:15:23Z

Which data member does the energy and pT (or eta or phi) correspond to?

Dr15Jones · 2022-08-09T16:29:12Z

So PFCandidate::energy() is read from LeafCandidate::m_state:: p4Cartesian_ where that member variable is declared transient to ROOT.

cmssw/DataFormats/Candidate/src/classes_def.xml

Line 7 in e52f99a

the value is supposed to be filled by the IO rule

cmssw/DataFormats/Candidate/src/classes_def.xml

Lines 9 to 12 in e52f99a

    
           <!--The 'int pdgId_' is a workaround for ROOT bug which affects iorules for split classes--> 
        
           <ioread sourceClass = "reco::ParticleState" version="[1-]" targetClass="reco::ParticleState" source="ROOT::Math::LorentzVector<ROOT::Math::PtEtaPhiM4D<double> > p4Polar_;int pdgId_" target="p4Cartesian_"> 
        
            <![CDATA[newObj->setCartesian();]]> 
        
           </ioread>

So it appears that when schema evolution is being used, that the necessary IO rules are not always being run.

Dr15Jones · 2022-08-09T16:34:02Z

So pT, eta and phi come from LeafCandidate::m_state::p4Polar_ which is stored.

Dr15Jones · 2022-08-09T16:38:34Z

So if I do

[cdj@cmslpc-c8-heavy01 matrix]$ root -l root://eoscms.cern.ch//eos/cms/store/user/cmsbuild/store/data/Run2017F/JetHT/AOD/17Nov2017-v1/70000/0054D144-36DF-E711-B074-02163E01A47B.root
root [1] Events->Scan("recoPFCandidates_particleFlow__RECO.obj.m_state.energy()")

using the original format I get

***********************************
*    Row   * Instance * recoPFCan *
***********************************
*        0 *        0 * 0.8107478 *
*        0 *        1 * 1.5003893 *
*        0 *        2 * 0.5893012 *
*        0 *        3 * 1.2443912 *
*        0 *        4 * 9.3277593 *
*        0 *        5 * 2.9629352 *
*        0 *        6 * 3.7514724 *
*        0 *        7 * 2.8305396 *
*        0 *        8 * 1.6818398 *
*        0 *        9 * 1.7144627 *
*        0 *       10 * 0.9533503 *
*        0 *       11 * 7.2343502 *
*        0 *       12 * 5.6262460 *
*        0 *       13 * 0.9747063 *
*        0 *       14 * 9.3023527 *
*        0 *       15 * 1.4282982 *
*        0 *       16 * 4.3854562 *
*        0 *       17 * 1.2034772 *
*        0 *       18 * 1.9004409 *
*        0 *       19 * 1.6507877 *
*        0 *       20 * 1.9658588 *
*        0 *       21 * 1.9287844 *
*        0 *       22 * 2.9119759 *
*        0 *       23 * 0.7939654 *
*        0 *       24 * 0.6640725 *

but using the new format I get

***********************************
*    Row   * Instance * recoPFCan *
***********************************
*        0 *        0 *         0 *
*        0 *        1 *         0 *
*        0 *        2 * 2.84e-306 *
*        0 *        3 *         0 *
*        0 *        4 *         0 *
*        0 *        5 *         0 *
*        0 *        6 *         0 *
*        0 *        7 *         0 *
*        0 *        8 * 2.84e-306 *
*        0 *        9 * 2.84e-306 *
*        0 *       10 * 2.84e-306 *
*        0 *       11 *         0 *
*        0 *       12 *         0 *
*        0 *       13 * 2.84e-306 *
*        0 *       14 *         0 *
*        0 *       15 * 2.84e-306 *
*        0 *       16 *         0 *
*        0 *       17 * 2.84e-306 *
*        0 *       18 *         0 *
*        0 *       19 * 2.84e-306 *
*        0 *       20 *         0 *
*        0 *       21 * 2.84e-306 *
*        0 *       22 *         0 *
*        0 *       23 * 3.72e-307 *
*        0 *       24 * 2.84e-306 *

Dr15Jones · 2022-08-09T16:40:24Z

I'm doing this using CMSSW_12_5_0_pre4 which is using a version of ROOT 6.24.07

pcanal · 2022-08-09T16:50:50Z

Fair enough. It seems to have to do with the handling/passing-through to the base class. Let me try to write a standalone reproducer.

pcanal · 2022-08-10T16:07:09Z

I can reproduce locally. I am looking for a solution.

Dr15Jones · 2023-11-20T19:39:32Z

@pcanal was this problem solved?

cmsbuild · 2024-02-06T10:09:52Z

Milestone for this pull request has been moved to CMSSW_14_1_X. Please open a backport if it should also go in to CMSSW_14_0_X.

vlimant · 2024-03-29T12:52:20Z

-xpog
can we close this and reopen when necessary ?

cmsbuild · 2024-08-27T08:08:49Z

Milestone for this pull request has been moved to CMSSW_14_2_X. Please open a backport if it should also go in to CMSSW_14_1_X.

antoniovilela · 2024-09-03T09:44:23Z

ping (to make bot change milestone)

PFCandidate no longer inherits from CompositeCandidate

85e0542

No code in CMSSW made use of the possible compositeness of the class.

cmsbuild added this to the CMSSW_12_5_X milestone Aug 8, 2022

cmsbuild added code-checks-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Aug 8, 2022

cmsbuild added code-checks-approved and removed code-checks-pending labels Aug 8, 2022

cmsbuild added tests-started and removed tests-pending labels Aug 8, 2022

cmsbuild added tests-rejected and removed tests-started labels Aug 8, 2022

smuzaffar modified the milestones: CMSSW_12_5_X, CMSSW_12_6_X Aug 28, 2022

cmsbuild mentioned this pull request Sep 12, 2022

Pf cand rescale #39368

Merged

cmsbuild modified the milestones: CMSSW_14_0_X, CMSSW_13_3_X Nov 6, 2023

nsmith- mentioned this pull request Nov 20, 2023

PFCandidate no longer inherits from CompositeCandidate cms-rntuple-work/cmssw#4

Closed

nsmith- mentioned this pull request Nov 22, 2023

PFCandidate no longer inherits from CompositeCandidate cms-rntuple-work/cmssw#8

Open

makortel mentioned this pull request Dec 6, 2023

Schema evolution failure when removing an intermediate base class #43516

Open

cmsbuild mentioned this pull request Dec 20, 2023

Soft muon MVA id for run3 #43622

Merged

cmsbuild modified the milestones: CMSSW_14_0_X, CMSSW_14_1_X Feb 6, 2024

cmsbuild added xpog-rejected and removed xpog-pending labels Mar 29, 2024

cmsbuild mentioned this pull request Jul 15, 2024

[WIP] Remove all std::pair<X, Y> dictionaries as unnecessary #45466

Closed

cmsbuild mentioned this pull request Aug 1, 2024

update PackedCandidate::dzError to return what it should in track parameterization #45612

Open

cmsbuild modified the milestones: CMSSW_14_1_X, CMSSW_14_2_X Aug 27, 2024

cmsbuild added the changes-dataformats label Aug 27, 2024

cmsbuild modified the milestones: CMSSW_14_1_X, CMSSW_14_2_X Sep 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PFCandidate no longer inherits from CompositeCandidate #38999

PFCandidate no longer inherits from CompositeCandidate #38999

Dr15Jones commented Aug 8, 2022

cmsbuild commented Aug 8, 2022

cmsbuild commented Aug 8, 2022 •

edited

Loading

Dr15Jones commented Aug 8, 2022

Dr15Jones commented Aug 8, 2022

cmsbuild commented Aug 8, 2022

Dr15Jones commented Aug 9, 2022

Dr15Jones commented Aug 9, 2022

pcanal commented Aug 9, 2022

Dr15Jones commented Aug 9, 2022

Dr15Jones commented Aug 9, 2022

Dr15Jones commented Aug 9, 2022

Dr15Jones commented Aug 9, 2022

pcanal commented Aug 9, 2022

pcanal commented Aug 10, 2022

Dr15Jones commented Nov 20, 2023

cmsbuild commented Feb 6, 2024

vlimant commented Mar 29, 2024

cmsbuild commented Aug 27, 2024

antoniovilela commented Sep 3, 2024

PFCandidate no longer inherits from CompositeCandidate #38999

Are you sure you want to change the base?

PFCandidate no longer inherits from CompositeCandidate #38999

Conversation

Dr15Jones commented Aug 8, 2022

PR description:

PR validation:

cmsbuild commented Aug 8, 2022

cmsbuild commented Aug 8, 2022 • edited Loading

Dr15Jones commented Aug 8, 2022

Dr15Jones commented Aug 8, 2022

cmsbuild commented Aug 8, 2022

Unit Tests

RelVals

RelVals-INPUT

AddOn Tests

Dr15Jones commented Aug 9, 2022

Dr15Jones commented Aug 9, 2022

pcanal commented Aug 9, 2022

Dr15Jones commented Aug 9, 2022

Dr15Jones commented Aug 9, 2022

Dr15Jones commented Aug 9, 2022

Dr15Jones commented Aug 9, 2022

pcanal commented Aug 9, 2022

pcanal commented Aug 10, 2022

Dr15Jones commented Nov 20, 2023

cmsbuild commented Feb 6, 2024

vlimant commented Mar 29, 2024

cmsbuild commented Aug 27, 2024

antoniovilela commented Sep 3, 2024

cmsbuild commented Aug 8, 2022 •

edited

Loading