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

[EGM HLT@Phase2] Phase out caloTowers #40525

Merged
merged 3 commits into from
Jan 16, 2023

Conversation

swagata87
Copy link
Contributor

PR description:

With this PR we propose to discontinue the usage of caloTowers in Phase2 E/gamma HLT.
This is done in a similar way as it was done for Run3 EGM HLT as described in [1], [2], [3], [4].

[1] https://its.cern.ch/jira/browse/CMSHLT-2286
[2] https://indico.cern.ch/event/1116957/contributions/4716661/attachments/2383419/4072743/caloTowerEGMHLT.pdf
[3] #35049
[4] #36157

PR validation:

RecHit-based H/E and CaloTower-based H/E are very close. This plot is made from CMSSW_12_4_8, using ZprimeToEE sample (PhaseIISpring22, PU200). The objects entering the plot are all the HLT-level superclusters with pT>10 GeV, in barrel.
Screen Shot 2023-01-13 at 23 15 11

Tagging HLT upgrade convenors @beaucero and @SohamBhattacharya

Backport of this PR is not necessary.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40525/33732

  • This PR adds an extra 36KB to repository

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @swagata87 (Swagata Mukherjee) for master.

It involves the following packages:

  • HLTrigger/Configuration (hlt)

@cmsbuild, @missirol, @Martin-Grunewald can you please review it and eventually sign? Thanks.
@Martin-Grunewald, @missirol, @silviodonato this is something you requested to watch as well.
@perrotta, @dpiparo, @rappoccio you are the release manager for this.

cms-bot commands are listed here

Copy link
Contributor

@missirol missirol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit confused.

Doesn't the change to hltFixedGridRhoFastjetAllCaloForMuons also affect the muon reconstruction?

If that is intended, and the same module is used by both EGM and MUO, the postfix "ForMuons" could be removed for clarity.

Several EGM modules (e.g. hltDiEG2312IsoHEL1SeededFilter) still reference hltFixedGridRhoFastjetAllCaloForEGamma (which doesn't exist); I guess it does not matter because doRhoCorrection = False, but this should be replaced with an empty InputTag (like in hltDiEG2312IsoClusterShapeL1SeededFilter), or the name of an appropriate (existing) collection (e.g. hltFixedGridRhoFastjetAllCalo(ForMuons)).

The files hltTowerMakerForAllForEgamma_cfi and hltRegionalTowerForEgamma_cfi should be removed if not used (unless there is a good reason to keep them).

Comment on lines 11 to 12
eThresHB = cms.vdouble( 0.1, 0.2, 0.3, 0.3 ),
eThresHE = cms.vdouble( 0.1, 0.2, 0.2, 0.2, 0.2, 0.2, 0.2 )
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These look like the Run-3 thresholds. Are they the same for Phase 2 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's double check this with @cms-sw/hcal-dpg-l2 and @cms-sw/pf-l2
Is the following still valid for phase2? Or do we have new PF recHit threshold for HB for phase2?

_thresholdsHBphase1 = cms.vdouble(0.1, 0.2, 0.3, 0.3)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought this depends on the 'aging' scenario, e.g.


, and it would match what is used in the modules below [*], where there are already some differences.

Indeed, it would be good to clarify this.

[*]


gatheringThreshold = cms.vdouble(0.8, 1.2, 1.2, 1.2),


Copy link

@abdoulline abdoulline Jan 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was about to point to usual Phase2 aging customization
https://github.com/cms-sw/cmssw/blob/master/SLHCUpgradeSimulations/Configuration/python/aging.py#245
With PFlowClusters thresholds/seeds involved.
But just noticed that Marino has already commented on it just several minutes ago (!)

Typical Phase2 use case is 1000/fb in this list:
https://github.com/cms-sw/cmssw/blob/master/SLHCUpgradeSimulations/Configuration/python/aging.py#L89-#L127

@swagata87
Copy link
Contributor Author

Doesn't the change to hltFixedGridRhoFastjetAllCaloForMuons also affect the muon reconstruction?

Indeed it does. But for now RhoCorrection is disabled in muon HLT also, so no difference will show up in tests.

If that is intended, and the same module is used by both EGM and MUO, the postfix "ForMuons" could be removed for clarity.

I have now kept hltFixedGridRhoFastjetAllCaloForMuons as it is and created a new file hltFixedGridRhoFastjetAllCaloForEGamma. This way, later on MUO POG can configure the hltFixedGridRhoFastjetAllCaloForMuons differently than egamma if they need to.

Several EGM modules (e.g. hltDiEG2312IsoHEL1SeededFilter) still reference hltFixedGridRhoFastjetAllCaloForEGamma (which doesn't exist); I guess it does not matter because doRhoCorrection = False, but this should be replaced with an empty InputTag (like in hltDiEG2312IsoClusterShapeL1SeededFilter), or the name of an appropriate (existing) collection (e.g. hltFixedGridRhoFastjetAllCalo(ForMuons)).

this issue is now solved as I have now introduced a new module named hltFixedGridRhoFastjetAllCaloForEGamma

The files hltTowerMakerForAllForEgamma_cfi and hltRegionalTowerForEgamma_cfi should be removed if not used (unless there is a good reason to keep them).

these 2 files are now removed

The latest changes are tested with runTheMatrix.py -l 20834.76

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40525/33733

  • This PR adds an extra 36KB to repository

@cmsbuild
Copy link
Contributor

Pull request #40525 was updated. @cmsbuild, @missirol, @Martin-Grunewald can you please check and sign again.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40525/33736

  • This PR adds an extra 40KB to repository

@cmsbuild
Copy link
Contributor

Pull request #40525 was updated. @cmsbuild, @missirol, @Martin-Grunewald can you please check and sign again.

@swagata87
Copy link
Contributor Author

thank you Marino and Salavat for pointing out the correct threshold values. I updated it now.
Due to higher values of thresholds the distributions of HCAL related quantities are now slightly shifted to lower values as expected. I don't think this is a problem, and if needed the cut values can be re-optimised at a later time.

In the following plots, Black is base, and Green is [base+PR]
H/E
Screen Shot 2023-01-16 at 09 36 52

HCAL PF cluster Isolation (this is unrelated to caloTower->RecHit change. This variable changed as I updated HCAL thresholds in hltParticleFlowRecHitHBHEForEgamma_cfi.py and hltParticleFlowClusterHBHEForEgamma_cfi.py.
Screen Shot 2023-01-16 at 09 41 12

I take note of the fact that there are many other differences between modules/particleFlowClusterHBHE_cfi.py and modules/hltParticleFlowClusterHBHEForEgamma_cfi.py. It looks like we could align these 2 modules, and the egamma version of the module needs some update. But it might need some more time to figure this out. If it's fine, it can be done in a followup PR.

@missirol
Copy link
Contributor

please test

If it's fine, it can be done in a followup PR.

That'd be okay with me.

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-cdd063/30000/summary.html
COMMIT: fda2b10
CMSSW: CMSSW_13_0_X_2023-01-15-2300/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/40525/30000/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 4 differences found in the comparisons
  • DQMHistoTests: Total files compared: 49
  • DQMHistoTests: Total histograms compared: 3555538
  • DQMHistoTests: Total failures: 6
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3555510
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
  • Checked 211 log files, 162 edm output root files, 49 DQM output files
  • TriggerResults: no differences found

@missirol
Copy link
Contributor

+hlt

@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. @perrotta, @dpiparo, @rappoccio (and backports should be raised in the release meeting by the corresponding L2)

@perrotta
Copy link
Contributor

+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.

5 participants