Reorganization of kdtrees for PFBlockAlgo and optimize track-hcal links #31295

hatakeyamak · 2020-08-31T05:04:02Z

PR description:

This PR re-organizes kdtrees for track-hcal and track-ecal links, and optimize the track-hcal link storage. It should give much more granular PFBlocks without changing PF outputs (with some caveats [a]), adding huge PFBlocks discussed at:
J. Pata (Sep 2019): https://indico.cern.ch/event/846887/contributions/3557300/
K. Hatakeyama (Mar 2020): https://indico.cern.ch/event/897397/contributions/3784811/

The PFBlock size change:

Original:
Offline: Eight PFBlocks with >1000 elements, rms of size 23.1
HLT: One PFBlock with >200 elements, rms of size 4.6

With this PR:
Offline: One PFBlocks with >1000 elements, rms of size 12.5
HLT: One PFBlock with >30 elements, rms of size ~1

igprof output with 2023PU ttbar (matrix 12634.99)

Rank    % total       Self       Self / Children   Function
[Original]
            0.3  .........       1.31 / 1.31         PFBlockProducer::produce(edm::Event&, edm::EventSetup const&) [815]
            0.1  .........       0.42 / 0.42         PFProducer::produce(edm::Event&, edm::EventSetup const&) [1774]
—>
[with this PR]
            0.2  .........       0.86 / 0.86         PFBlockProducer::produce(edm::Event&, edm::EventSetup const&) [1137]
            0.0  .........       0.12 / 0.12         PFProducer::produce(edm::Event&, edm::EventSetup const&) [3483]

I believe this speed-up mainly comes that now we are distinguishing two preshower layers at prefilter stage of linker. (not very time-consuming to start with in offline sequence, but this is a sanity check.)
(Original)
http://hep.baylor.edu/hatake/PFval/misc/igreport_perf_12634.99_PFBlockLinks_ref.res
(with this PR)
http://hep.baylor.edu/hatake/PFval/misc/igreport_perf_12634.99_PFBlockLinks_test.res

[a] Some changes are observed in, for example, rawCaloFraction and rawHcalFraction of packed candidates, while the caloFraction and hcalFraction stay the same. These "raw" quantities follow the history of how candidates are produced in PFBlocks, so this is not surprising.
Also, some changes in btag csv values are observed. In the past, when candidate order changes, we also observed that btag related quantities change. I believe this shouldn't have systematic bias toward the physics performance (to be confirmed by relval etc).

This PR also includes some cleanup, as a follow-up from #31151.

This PR also include a long-standing request from JME to have a config parameter to control the eta range for ECAL-HCAL linkings (currently performed only for |eta|>2.5 i.e. outside the traditional tracker acceptance). @lathomas @ahinzmann

This will have some interference with ongoing: #31191.
It probably makes sense that this simpler PR goes first.

@bendavid @jpata

PR validation:

Run the matrix test 11834.0 (ttbar 2021 with PU), make sure we don't see changes in PF outputs in hlt, reco, and miniaod.
Now also ran 23234.0_TTbar_14TeV+2026D49 and made sure reco PF candidates stay the same.

The PF validation based on 2021 samples can be found at:
http://hep.baylor.edu/hatake/PFval/PFValidation_PFBlockLinks/

if this PR is a backport please specify the original PR and why you need to backport that PR:

This is not backport.

…he number of max hcal links per track.

… for links

…d tracks, V0, and gammaconv tracks.

cmsbuild · 2020-08-31T05:04:31Z

The code-checks are being triggered in jenkins.

cmsbuild · 2020-08-31T05:06:31Z

The code-checks are being triggered in jenkins.

cmsbuild · 2020-08-31T05:08:21Z

The code-checks are being triggered in jenkins.

cmsbuild · 2020-08-31T05:15:59Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-31295/18022

This PR adds an extra 48KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File RecoParticleFlow/PFProducer/interface/KDTreeLinkerBase.h modified in PR(s): Readiness of PFBlockAlgo to heterogeneous computing: use SOA #31191
- File RecoParticleFlow/PFProducer/plugins/kdtrees/KDTreeLinkerTrackEcal.cc modified in PR(s): Readiness of PFBlockAlgo to heterogeneous computing: use SOA #31191
- File RecoParticleFlow/PFProducer/plugins/kdtrees/KDTreeLinkerTrackHcal.cc modified in PR(s): Readiness of PFBlockAlgo to heterogeneous computing: use SOA #31191
- File RecoParticleFlow/PFProducer/plugins/linkers/ECALAndHCALLinker.cc modified in PR(s): Readiness of PFBlockAlgo to heterogeneous computing: use SOA #31191
- File RecoParticleFlow/PFProducer/plugins/linkers/TrackAndECALLinker.cc modified in PR(s): Readiness of PFBlockAlgo to heterogeneous computing: use SOA #31191
- File RecoParticleFlow/PFProducer/plugins/linkers/TrackAndHCALLinker.cc modified in PR(s): Readiness of PFBlockAlgo to heterogeneous computing: use SOA #31191

cmsbuild · 2020-08-31T05:16:23Z

A new Pull Request was created by @hatakeyamak (Kenichi Hatakeyama) for master.

It involves the following packages:

DataFormats/ParticleFlowReco
HLTrigger/Configuration
RecoParticleFlow/PFClusterProducer
RecoParticleFlow/PFClusterTools
RecoParticleFlow/PFProducer

@perrotta, @Martin-Grunewald, @slava77, @cmsbuild, @fwyzard, @jpata can you please review it and eventually sign? Thanks.
@mmarionncern, @makortel, @felicepantaleo, @riga, @rovere, @lgray, @Martin-Grunewald, @clelange, @lecriste, @cbernet, @seemasharmafnal this is something you requested to watch as well.
@silviodonato, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

jpata · 2020-08-31T05:51:06Z

@cmsbuild please test

cmsbuild · 2020-08-31T05:51:28Z

The tests are being triggered in jenkins.

CMSSW_11_2_X_2020-08-30-2300/slc7_amd64_gcc820: https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/9009/console Started: 2020/08/31 07:53

cmsbuild · 2020-08-31T07:07:03Z

-1

Tested at: ba64a58

CMSSW: CMSSW_11_2_X_2020-08-30-2300
SCRAM_ARCH: slc7_amd64_gcc820
You can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-e22797/9009/summary.html

I found follow errors while testing this PR

Failed tests: RelVals

RelVals:

When I ran the RelVals I found an error in the following workflows:
23234.0 step3

runTheMatrix-results/23234.0_TTbar_14TeV+2026D49+TTbar_14TeV_TuneCP5_GenSimHLBeamSpot14+DigiTrigger+RecoGlobal+HARVESTGlobal/step3_TTbar_14TeV+2026D49+TTbar_14TeV_TuneCP5_GenSimHLBeamSpot14+DigiTrigger+RecoGlobal+HARVESTGlobal.log

28234.0 step3

runTheMatrix-results/28234.0_TTbar_14TeV+2026D60+TTbar_14TeV_TuneCP5_GenSimHLBeamSpot14+DigiTrigger+RecoGlobal+HARVESTGlobal/step3_TTbar_14TeV+2026D60+TTbar_14TeV_TuneCP5_GenSimHLBeamSpot14+DigiTrigger+RecoGlobal+HARVESTGlobal.log

23434.999 step4

runTheMatrix-results/23434.999_TTbar_14TeV+2026D49PU_PMXS1S2PR+TTbar_14TeV_TuneCP5_GenSimHLBeamSpot14+PREMIX_PremixHLBeamSpot14PU+DigiTriggerPU+RecoGlobalPU+HARVESTGlobalPU/step4_TTbar_14TeV+2026D49PU_PMXS1S2PR+TTbar_14TeV_TuneCP5_GenSimHLBeamSpot14+PREMIX_PremixHLBeamSpot14PU+DigiTriggerPU+RecoGlobalPU+HARVESTGlobalPU.log

cmsbuild · 2020-08-31T07:07:04Z

Comparison not run due to runTheMatrix errors (RelVals and Igprof tests were also skipped)

hatakeyamak · 2020-09-13T01:56:50Z

Do you have any more info on this one?

while this change is much smaller in the QCD noPU sample:
http://hep.baylor.edu/hatake/PFval/PFValidation_PFBlockLinks/FlatQCD_NoPU_2021_ParticleFlow/PackedCandidates/electron/electronEta.pdf
Trying to see if this is the desired change.

This has been investigated and I think actually changes are favored ones. I will try to summarize that over the next couple of days.

Since the largest changes appeared in electrons, I looked at gedGefElectrons (well, actually slimmedElectrons in MINIAODSIM) more closely. The above plot comes from QCD,so the reduction of electron yields is likely the reduction of fakes, but another question is what would be the impact on real electrons. So, I looked at ZEE PU 2021 relval samples (slimmedElectrons, no ID on top of it):
http://hep.baylor.edu/hatake/PFval/PFValidation_PFBlockLinks/SlimmedElectronEta_PFBlockLinks.png
which shows that electrons matched to gen-electrons are basically fully maintained, while the reduction appears in those which don't match to gen-electrons.

This is probably due to reduced combinatorics in PFEGammaAlgo, thanks to more granular PFBlock sizes. So, it looks like this is a favored changes as I already commented.

This might also indicates that more close review of PFEGammaAlgo and how links are handled can improve its performance a little more.

To me this PR looks good to go.

@jainshilpi (any other EGamma people should be notified?)

Martin-Grunewald · 2020-09-14T10:53:22Z

+1

cmsbuild · 2020-09-14T10:53:47Z

This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @silviodonato, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

silviodonato · 2020-09-15T08:42:09Z

+1

Dr15Jones · 2020-09-15T15:13:48Z

This appears to have broken today's IB. The problem is

----- Begin Fatal Exception 15-Sep-2020 12:42:37 CEST-----------------------
An exception of category 'FileReadError' occurred while
   [0] Rethrowing an exception that happened on a different thread.
   [1] Reading branch recoPFBlocks_particleFlowBlock__RECO.
   Additional Info:
      [a] Fatal Root Error: @SUB=TStreamerInfo::BuildOld
Cannot convert reco::PFBlockElement::multilinks_ from type: reco::PFMultiLinksTC to type: map<reco::PFBlockElement::Type,reco::PFMultiLinksTC>, skip element

----- End Fatal Exception -------------------------------------------------

so we can no longer read the previously written std::vector<reco::PFBlock> data.

Another item of note is std::map<> are very bad for ROOT storage. They are not handled particularly well which means they take up lots of space on disk and are slow to read/write.

In addition std::map<> has a poor in memory layout as each item in the map has its own memory allocation. This requires a large number of memory reads to move around in the map. The best use of std::map<> is where you are doing large number of inserts and removes from the data structure and need the structure to remain sorted the entire time. For storage, that is not the case, we write it once and read many times.

A better data structure for an Event data product would be a sorted std::vector<std::pair<>>. It stores well and it has a very compressed memory structure.

hatakeyamak · 2020-09-15T15:35:16Z

Another item of not is std::map<> are very bad for ROOT storage. They are not handled particularly well which means they take up lots of space on disk and are slow to read/write.

In addition std::map<> has a poor in memory layout as each item in the map has its own memory allocation. This requires a large number of memory reads to move around in the map. The best use of std::map<> is where you are doing large number of inserts and removes from the data structure and need the structure to remain sorted the entire time. For storage, that is not the case, we write it once and read many times.

A better data structure for an Event data product would be a sorted std::vector<std::pair<>>. It stores well and it has a very compressed memory structure.

I see. Thank you. I will look intostd::vector<std::pair<>>.

so we can no longer read the previously written std::vector<reco::PFBlock> data.

I see. I am not clear what we should do about it, and I will appreciate suggestions. Sorry for the trouble.

makortel · 2020-09-15T15:39:53Z

so we can no longer read the previously written std::vector<reco::PFBlock> data.

I see. I am not clear what we should do about it, and I will appreciate suggestions. Sorry for the trouble.

Can the earlier "scalar" PFMultiLinksTC multilinks_ be read into one element of the std::vector<std::pair<reco::PFBlockElement::Type, PFMultiLinksTC>> aggregate? If yes, it could be dealt with an ioread rule.

hatakeyamak · 2020-09-15T16:00:30Z

I see. Maybe we can make it work so that it won't crash, but I am not sure if it will do right thing, given that how PFMultiLinksTC is used for different combinations of links is being changed.
What workflow/test reads previously-produced std::vectorreco::PFBlock?

Dr15Jones · 2020-09-15T16:05:06Z

Workflows: 134.807, 134.808, 134.907, 134.908, 136.727, 136.728 and 136.8391 are the ones failing in the IB all in step 2.

Dr15Jones · 2020-09-15T16:05:33Z

See https://cmssdt.cern.ch/SDT/html/cmssdt-ib/#/relVal/CMSSW_11_2/2020-09-15-1100?selectedArchs=slc7_amd64_gcc820&selectedFlavors=X&selectedStatus=failed

hatakeyamak · 2020-09-15T16:13:14Z

Thank you. I will have a deeper look with urgency.

slava77 · 2020-09-15T16:20:52Z

134.807

this is reading /DoubleEG/Run2015C-ZElectron-PromptReco-v1/RAW-RECO
Is the PFBlock actually consumed or is this an error due to simply reading the file?
For the latter, we could as well come up with some solution that just ignores this data in the input.

hatakeyamak · 2020-09-15T16:25:14Z

I am hoping the latter is the case, but I don't know. I will attack this after my class... Of course I will be happy if somebody come up with a solution by then.

hatakeyamak · 2020-09-16T01:34:12Z

So, if I comment out:
https://cmssdt.cern.ch/lxr/source/RecoParticleFlow/Configuration/python/RecoParticleFlow_EventContent_cff.py#0066
'keep recoPFBlocks_particleFlowBlock_*_*',
then 134.807 works fine, so I think this block is not really consumed.
In this situation, I am still wondering what would be the best way forward.

slava77 · 2020-09-16T03:31:30Z

So, if I comment out:
https://cmssdt.cern.ch/lxr/source/RecoParticleFlow/Configuration/python/RecoParticleFlow_EventContent_cff.py#0066
'keep recoPFBlocks_particleFlowBlock_*_*',
then 134.807 works fine, so I think this block is not really consumed.
In this situation, I am still wondering what would be the best way forward.

if the input multilinks_ is not really needed, then one solution is an ioread rule (in src/*_def.xml file) to specify that the new object multilinks is initialized to an empty/default-constructed value.
I'd still suggest to try to read it properly, if the conversion is not too complicated.

jpata · 2020-09-16T07:26:37Z

I'm a bit concerned that this bug did not show up in the jenkins tests. How can we improve this?

jpata · 2020-09-16T12:45:12Z

It does not seem like the multilinks are used anywhere outside of PFBlockAlgo. As they are created within PFBlockAlgo for internal consumption, how about marking them transient in the future (or otherwise not saving them in the EDM file, also to save disk space), and just initializing them with defaults to prevent a crash in case of reading legacy files.

Just to note, in #31191 (a rebase wrt. this PR is almost done) we change multilinks to be index-based, so this can be revisited once more.

Dr15Jones · 2020-09-16T13:10:00Z

@jpata

I'm a bit concerned that this bug did not show up in the jenkins tests. How can we improve this?

For the framework, we have ROOT files containing RAW data for all the different file 'format's we've had throughout the history of CMSSW. We have unit tests which run over those files to be sure we can always read the old formats. We have this since the framework guarantees that we can always read all old RAW data files.

We don't have anything similar for files containing other data formats.

jpata · 2020-09-16T13:14:24Z

@Dr15Jones

We don't have anything similar for files containing other data formats.

OK, understood. Maybe we can look increasing the unit test coverage for reading old files also for reco.

In order to fix the issue arising in the IB now, I tried the following: #31485, perhaps we can address that particular approach there.

hatakeyamak · 2020-09-16T14:25:23Z

@jpata thx for #31485. I already considered possibly making multilinks transient in this PR, but didn't go ahead without seeing a real need, but I think this is good.

Maybe one of RAW-RECO workflow can be made as a part of standard jenkins test to catch this sort of things in future.

Dr15Jones · 2020-09-16T14:28:28Z

Maybe one of RAW-RECO workflow can be made as a part of standard jenkins test to catch this sort of things in future.

A simpler idea would be to just have a job that only reads/writes a file (forcing it not to do a 'fast copy').

hatakeyamak added 5 commits August 27, 2020 08:21

Re-organize the track-hcal and track-ecal kdtrees. Allow specifying t…

c4abf6a

…he number of max hcal links per track.

cleanup

c9d6755

Add minAbsEtaEcal for controlling the eta range for ECAL-HCAL links

38cf211

Introduce a const minPFRecHitFrac for controlling the min rechit frac…

1f8dc5b

… for links

Exclude the requirement of the max hcal links per track from displace…

84ca2c5

…d tracks, V0, and gammaconv tracks.

cmsbuild added this to the CMSSW_11_2_X milestone Aug 31, 2020

cmsbuild added code-checks-pending comparison-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Aug 31, 2020

PR31295 for HLT configs.

3aceffa

cmsbuild added the hlt-pending label Aug 31, 2020

Merge branch 'master' into KDTreeTracckHcalEcalLinkOptimization

ba64a58

hatakeyamak changed the title ~~Reorganization of kdtrees and optimize track-hcal links~~ Reorganization of kdtrees for PFBlockAlgo and optimize track-hcal links Aug 31, 2020

cmsbuild added code-checks-approved and removed code-checks-pending labels Aug 31, 2020

cmsbuild added tests-started and removed tests-pending labels Aug 31, 2020

cmsbuild removed the comparison-pending label Aug 31, 2020

cmsbuild added the reconstruction-approved label Sep 11, 2020

cmsbuild added fully-signed hlt-approved and removed hlt-pending pending-signatures labels Sep 14, 2020

cmsbuild added orp-approved and removed orp-pending labels Sep 15, 2020

cmsbuild merged commit c24fc31 into cms-sw:master Sep 15, 2020

jpata mentioned this pull request Sep 16, 2020

make multilinks transient #31485

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reorganization of kdtrees for PFBlockAlgo and optimize track-hcal links #31295

Reorganization of kdtrees for PFBlockAlgo and optimize track-hcal links #31295

hatakeyamak commented Aug 31, 2020 •

edited

cmsbuild commented Aug 31, 2020

cmsbuild commented Aug 31, 2020

cmsbuild commented Aug 31, 2020

cmsbuild commented Aug 31, 2020

cmsbuild commented Aug 31, 2020

jpata commented Aug 31, 2020

cmsbuild commented Aug 31, 2020 •

edited

cmsbuild commented Aug 31, 2020

cmsbuild commented Aug 31, 2020

hatakeyamak commented Sep 13, 2020 •

edited

Martin-Grunewald commented Sep 14, 2020

cmsbuild commented Sep 14, 2020

silviodonato commented Sep 15, 2020

Dr15Jones commented Sep 15, 2020 •

edited

hatakeyamak commented Sep 15, 2020 •

edited

makortel commented Sep 15, 2020

hatakeyamak commented Sep 15, 2020 •

edited

Dr15Jones commented Sep 15, 2020

Dr15Jones commented Sep 15, 2020

hatakeyamak commented Sep 15, 2020

slava77 commented Sep 15, 2020

hatakeyamak commented Sep 15, 2020

hatakeyamak commented Sep 16, 2020 •

edited

slava77 commented Sep 16, 2020

jpata commented Sep 16, 2020

jpata commented Sep 16, 2020 •

edited

Dr15Jones commented Sep 16, 2020

jpata commented Sep 16, 2020

hatakeyamak commented Sep 16, 2020

Dr15Jones commented Sep 16, 2020

Reorganization of kdtrees for PFBlockAlgo and optimize track-hcal links #31295

Reorganization of kdtrees for PFBlockAlgo and optimize track-hcal links #31295

Conversation

hatakeyamak commented Aug 31, 2020 • edited

PR description:

PR validation:

if this PR is a backport please specify the original PR and why you need to backport that PR:

cmsbuild commented Aug 31, 2020

cmsbuild commented Aug 31, 2020

cmsbuild commented Aug 31, 2020

cmsbuild commented Aug 31, 2020

cmsbuild commented Aug 31, 2020

jpata commented Aug 31, 2020

cmsbuild commented Aug 31, 2020 • edited

cmsbuild commented Aug 31, 2020

cmsbuild commented Aug 31, 2020

hatakeyamak commented Sep 13, 2020 • edited

Martin-Grunewald commented Sep 14, 2020

cmsbuild commented Sep 14, 2020

silviodonato commented Sep 15, 2020

Dr15Jones commented Sep 15, 2020 • edited

hatakeyamak commented Sep 15, 2020 • edited

makortel commented Sep 15, 2020

hatakeyamak commented Sep 15, 2020 • edited

Dr15Jones commented Sep 15, 2020

Dr15Jones commented Sep 15, 2020

hatakeyamak commented Sep 15, 2020

slava77 commented Sep 15, 2020

hatakeyamak commented Sep 15, 2020

hatakeyamak commented Sep 16, 2020 • edited

slava77 commented Sep 16, 2020

jpata commented Sep 16, 2020

jpata commented Sep 16, 2020 • edited

Dr15Jones commented Sep 16, 2020

jpata commented Sep 16, 2020

hatakeyamak commented Sep 16, 2020

Dr15Jones commented Sep 16, 2020

hatakeyamak commented Aug 31, 2020 •

edited

cmsbuild commented Aug 31, 2020 •

edited

hatakeyamak commented Sep 13, 2020 •

edited

Dr15Jones commented Sep 15, 2020 •

edited

hatakeyamak commented Sep 15, 2020 •

edited

hatakeyamak commented Sep 15, 2020 •

edited

hatakeyamak commented Sep 16, 2020 •

edited

jpata commented Sep 16, 2020 •

edited