Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switching HGCal electrons and photons to TICL-based reconstruction #31527

Merged
merged 8 commits into from Sep 30, 2020

Conversation

SohamBhattacharya
Copy link
Contributor

PR description:

  1. HGCal electron (and photon) sequence modified to use TICL as input, instead of the old Multiclusters.
    In addition, now there is an option to filter the input TICL tracksters based on their PID. This is turned off for now, but will be switched on soon in the future.
    Files affected:
    RecoParticleFlow/PFClusterProducer/python/particleFlowClusterHGC_cfi.py
    RecoParticleFlow/PFClusterProducer/plugins/PFClusterFromHGCalMultiCluster.cc

  2. Have turned off the old (default) track-based correction for electrons by adding a configurable boolean "useDefaultEnergyCorrection".
    Files affected:
    RecoEgamma/EgammaElectronProducers/plugins/GsfElectronProducer.cc
    RecoEgamma/EgammaElectronAlgos/interface/GsfElectronAlgo.h
    RecoEgamma/EgammaElectronAlgos/src/GsfElectronAlgo.cc

  3. TICL-based electron customization (like ambiguity resolution).
    Files affected:
    RecoEgamma/EgammaElectronProducers/python/gsfElectrons_cfi.py

PR validation:

  1. Test:
    scram b distclean
    git cms-checkdeps -a -A
    scram b -j 8
    scram b runtests

Outcome: OK.

  1. Test:
    runTheMatrix.py -l limited -i all --ibeos

Outcome:
35 23 22 15 8 1 0 0 0 tests passed, 0 11 0 0 0 0 0 0 0 failed

  1. Test:
    scram build code-checks

Outcome: OK.

  1. Test:
    scram build code-format

Outcome: OK.

…instead of the old Multiclusters.

In addition, now there is an option to filter the TICL tracksters based on their PID.
This is turned off for now, but will be switched on soon in the future.
Files affected:
RecoParticleFlow/PFClusterProducer/python/particleFlowClusterHGC_cfi.py
RecoParticleFlow/PFClusterProducer/plugins/PFClusterFromHGCalMultiCluster.cc

2. Have turned off the old (default) track-based correction for electrons by adding a configurable boolean "useDefaultEnergyCorrection".
Files affected:
RecoEgamma/EgammaElectronProducers/plugins/GsfElectronProducer.cc
RecoEgamma/EgammaElectronAlgos/interface/GsfElectronAlgo.h
RecoEgamma/EgammaElectronAlgos/src/GsfElectronAlgo.cc

3. TICL-based electron customization (like ambiguity resolution).
Files affected:
RecoEgamma/EgammaElectronProducers/python/gsfElectrons_cfi.py
@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-31527/18494

  • This PR adds an extra 40KB to repository

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @SohamBhattacharya for master.

It involves the following packages:

RecoEgamma/EgammaElectronAlgos
RecoEgamma/EgammaElectronProducers
RecoParticleFlow/PFClusterProducer

@perrotta, @jpata, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@mmarionncern, @Sam-Harper, @felicepantaleo, @riga, @afiqaize, @varuns23, @rovere, @lgray, @sobhatta, @clelange, @lecriste, @hatakeyamak, @jainshilpi, @seemasharmafnal, @cbernet 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

@perrotta
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 21, 2020

The tests are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

-1

Tested at: 2447357

CMSSW: CMSSW_11_2_X_2020-09-21-1100
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-15bf68/9460/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:
136.874 step2

runTheMatrix-results/136.874_RunEGamma2018C+RunEGamma2018C+HLTDR2_2018+RECODR2_2018reHLT_skimEGamma_Offline_L1TEgDQM+HARVEST2018_L1TEgDQM/step2_RunEGamma2018C+RunEGamma2018C+HLTDR2_2018+RECODR2_2018reHLT_skimEGamma_Offline_L1TEgDQM+HARVEST2018_L1TEgDQM.log

4.22 step2
runTheMatrix-results/4.22_RunCosmics2011A+RunCosmics2011A+RECOCOSD+ALCACOSD+SKIMCOSD+HARVESTDC/step2_RunCosmics2011A+RunCosmics2011A+RECOCOSD+ALCACOSD+SKIMCOSD+HARVESTDC.log

136.88811 step2
runTheMatrix-results/136.88811_RunJetHT2018D_reminiaodUL+RunJetHT2018D_reminiaodUL+REMINIAOD_data2018UL+HARVEST2018_REMINIAOD_data2018UL/step2_RunJetHT2018D_reminiaodUL+RunJetHT2018D_reminiaodUL+REMINIAOD_data2018UL+HARVEST2018_REMINIAOD_data2018UL.log

4.53 step2
runTheMatrix-results/4.53_RunPhoton2012B+RunPhoton2012B+HLTD+RECODR1reHLT+HARVESTDR1reHLT/step2_RunPhoton2012B+RunPhoton2012B+HLTD+RECODR1reHLT+HARVESTDR1reHLT.log

136.793 step2
runTheMatrix-results/136.793_RunDoubleEG2017C+RunDoubleEG2017C+HLTDR2_2017+RECODR2_2017reHLT_skimDoubleEG_Prompt+HARVEST2017/step2_RunDoubleEG2017C+RunDoubleEG2017C+HLTDR2_2017+RECODR2_2017reHLT_skimDoubleEG_Prompt+HARVEST2017.log

@cmsbuild
Copy link
Contributor

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

@SohamBhattacharya
Copy link
Contributor Author

I see that the tests that have failed are due to file open errors.

@jpata
Copy link
Contributor

jpata commented Sep 28, 2020

The last minor update 3c675cb looks fine, as is expected.

@kpedro88
Copy link
Contributor

Has there been a high-statistics validation of this change? There aren't enough recoPhotons and recoGsfElectrons in the PR comparison to draw significant conclusions, but the PF cluster changes are concerning:
all_OldVSNew_TTbar14TeV2026D49wf23234p0c_log10recoPFClusters_particleFlowClusterHGCalFromMultiCl__RECO_obj_energy
all_OldVSNew_TTbar14TeV2026D49wf23234p0c_recoPFClusters_particleFlowClusterHGCalFromMultiCl__RECO_obj_eta

@jpata
Copy link
Contributor

jpata commented Sep 29, 2020

@SohamBhattacharya can you please clarify the last question? From my/reco point of view, it was understood that this is an incremental development change, so as long as there is no regression in standard (non-HGCAL) reco, this would be fine.

However, we can set the criterion (or perhaps it already exists and I'm not aware of it) that also upgrade reconstruction should not suffer regressions in the course of development.

@kpedro88
Copy link
Contributor

Changes in upgrade reco are acceptable if they're intentional.

@SohamBhattacharya
Copy link
Contributor Author

SohamBhattacharya commented Sep 29, 2020

@kpedro88 @jpata
Sorry for the late reply -- I was performing a few checks.
The TICL-based electrons have been studied in quite some detail in 11_1, and found to be working reasonably well -- a summary of the studies can be found in [1].
Following this there has been a general desire to switch to the TICL-based reconstruction as that will also make future testing and development a lot easier, especially in view of the HLT-TDR studies (for which there will be a backport to 11_1).

[1] https://indico.cern.ch/event/954335/contributions/4019355/attachments/2102621/3535270/presentation_20200915_HLTupgrade.pdf

As for this PR, here are the plots of TICL-based electron multiplicity and energy response in a flat-pT (0-200 GeV) electron-gun sample at 0 PU.
gsfEleFromTICL-n_SingleElectron_PU0_11_2_0_pre6
gsfEleFromTICL-E_by_genEle-E_SingleElectron_PU0_11_2_0_pre6

Similarly, here are the corresponding plots for the TICL-based photons in a flat-pT gun sample.
phoFromTICL-n_SinglePhoton_PU0_11_2_0_pre6
phoFromTICL-E_by_genPho-E_SinglePhoton_PU0_11_2_0_pre6

It can be seen that both the multiplicity and the response look reasonable at 0 PU in 11_2.
The 200PU scenario in 11_2 is still a work in progress, but as mentioned before, switching to a TICL-based reconstruction will help simplify the ongoing studies and optimizations.

@kpedro88
Copy link
Contributor

@SohamBhattacharya thanks for these studies. Can you provide a bit more information:

  • what the electron and photon response looked like before this change
  • if the PFCluster changes above are also expected/desirable

@SohamBhattacharya
Copy link
Contributor Author

@kpedro88 Here is the electron response before this change.
gsfEleFromMultiClus-E_by_genEle-E_SingleElectron_PU0_11_2_0_pre6

And this is the corresponding plot for photons.
phoFromMultiClus-E_by_genPho-E_SinglePhoton_PU0_11_2_0_pre6

As for the changes to "particleFlowClusterHGCalFromMultiCl" I don't see it as a problem as such. If I'm not mistaken, these are used for e-gamma reconstruction alone, and if that works alright, then the changes to "particleFlowClusterHGCalFromMultiCl" should be acceptable.
Now as for the specific differences that you've pointed out, @rovere and @felicepantaleo can answer better as each of these PFClusters is essentially a TICL trackster.
But looking at those plots I would naively guess that one reason might be TICL is more robust against pileup and is hence reconstructing fewer low energy clusters and also fewer clusters especially at higher eta.

@kpedro88
Copy link
Contributor

thanks; electron response definitely improves, while photon response seems to stay mostly the same.

@kpedro88
Copy link
Contributor

+upgrade

@jpata
Copy link
Contributor

jpata commented Sep 30, 2020

+reconstruction

  • switches HGCAL electrons and photons to TICL reconstruction
  • as expected, there are reco changes only to HGCAL electrons and photons
  • the physics performance changes to the latter have been documented above, more extensive studies with PU will continue
  • further cleaning of HGCAL paths from now unused modules is expected per Switching HGCal electrons and photons to TICL-based reconstruction #31527 (comment)
  • computational performance has been assessed, with CkfTrackCandidateMakerBase going up by ~0.25%

@cmsbuild
Copy link
Contributor

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)

@qliphy
Copy link
Contributor

qliphy commented Sep 30, 2020

+1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants