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

Migrate calotower code to use HCAL thresholds from GT #43329

Merged
merged 2 commits into from
Dec 4, 2023

Conversation

swagata87
Copy link
Contributor

PR description:

While our final aim is to fully deprecate calotowers, there is no clear ETA for that. Several POGs still use calotowers, specially at HLT. So, to ensure that correct HCAL thresholds are used in making calotowers, the safest way is to use the thresholds from GT.
That is what is done in this PR. Related github issue: #43312

This is a technical PR. In principle there should not be any change in physics. But I expect there will be some changes.

For Phase2 HLT, there might be some changes seen in PR tests, because the thresholds from DB and thresholds written here would not match:

HBThreshold = cms.double(1.2),
HBThreshold1 = cms.double(0.8),
HBThreshold2 = cms.double(1.2),

HBThreshold = cms.double(1.2),
HBThreshold1 = cms.double(0.8),
HBThreshold2 = cms.double(1.2),

In offline reco also, some changes can be seen in barrel for Run3, because the correct thresholds present in DB and thresholds written here does not seem to match:

run3_HB.toModify(calotowermaker,
HBThreshold1 = 0.1,
HBThreshold2 = 0.2,
HBThreshold = 0.3,

PR validation:

Checked with 12434.0_TTbar_14TeV+2023

FYI @missirol @cms-sw/hlt-l2 @cms-sw/alca-l2 @cms-sw/hcal-dpg-l2

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-43329/37779

  • This PR adds an extra 40KB to repository

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

  • RecoLocalCalo/CaloTowersCreator (reconstruction)

@mandrenguyen, @cmsbuild, @jfernan2 can you please review it and eventually sign? Thanks.
@youyingli, @apsallid, @missirol this is something you requested to watch as well.
@rappoccio, @antoniovilela, @sextonkennedy you are the release manager for this.

cms-bot commands are listed here

@swagata87
Copy link
Contributor Author

please test with #43305

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-2e268c/35947/summary.html
COMMIT: 5939338
CMSSW: CMSSW_14_0_X_2023-11-19-0000/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/43329/35947/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • You potentially added 26 lines to the logs
  • Reco comparison results: 985 differences found in the comparisons
  • DQMHistoTests: Total files compared: 50
  • DQMHistoTests: Total histograms compared: 3363868
  • DQMHistoTests: Total failures: 7053
  • DQMHistoTests: Total nulls: 3
  • DQMHistoTests: Total successes: 3356790
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.02 KiB( 49 files compared)
  • DQMHistoSizes: changed ( 141.044 ): 0.020 KiB JetMET/SUSYDQM
  • Checked 214 log files, 167 edm output root files, 50 DQM output files
  • TriggerResults: no differences found

@mmusich
Copy link
Contributor

mmusich commented Nov 19, 2023

For Phase2 HLT, there might be some changes seen in PR tests, because the thresholds from DB and thresholds written here would not match

Is that out of sloppiness in updating the configurations or is there some deeper reason?

@rovere @SohamBhattacharya FYI

@swagata87
Copy link
Contributor Author

Is that out of sloppiness in updating the configurations or is there some deeper reason?

This is not sloppiness. In those Phase2 HLT configs people have written the thresholds corresponding to 1000 fb-1 ageing, which is the most commonly used ageing scenario for studying physics in Phase2. But for some reasons, PR tests are done without any ageing; and in that special case one should fall back to Run3 thresholds, but that is not happening here for config-based approach, but happens for GT-based approach. It seem to be similar issue like the one discussed here: #43025 (comment) , so to fix it one should write Run3 thresholds in Phase HLT config, and if one uses proper ageing then those will be overwritten via

hcal_lumis = [300, 1000, 3000, 4500, 1e10]
hcal_thresholds = {
300: {
"seed": [0.5, 0.625, 0.75, 0.75],
"rec": [0.4, 0.5, 0.6, 0.6],
},
1000: {
"seed": [1.0, 1.5, 1.5, 1.5],
"rec": [0.8, 1.2, 1.2, 1.2],
},
3000: {
"seed": [1.25, 2.5, 2.5, 2.5],
"rec": [1.0, 2.0, 2.0, 2.0],
},
4500: {
"seed": [1.5, 3.0, 3.0, 3.0],
"rec": [1.25, 2.5, 2.5, 2.5],
},
}
ctmodules = ['calotowermaker','caloTowerForTrk','caloTowerForTrkPreSplitting','towerMaker','towerMakerWithHO']
for ilumi, hcal_lumi in enumerate(hcal_lumis[:-1]):
if lumi >= hcal_lumi and lumi < hcal_lumis[ilumi+1]:
if hasattr(process,'particleFlowClusterHBHE'):
process.particleFlowClusterHBHE.seedFinder.thresholdsByDetector[0].seedingThreshold = hcal_thresholds[hcal_lumi]["seed"]
process.particleFlowClusterHBHE.initialClusteringStep.thresholdsByDetector[0].gatheringThreshold = hcal_thresholds[hcal_lumi]["rec"]
process.particleFlowClusterHBHE.pfClusterBuilder.recHitEnergyNorms[0].recHitEnergyNorm = hcal_thresholds[hcal_lumi]["rec"]
process.particleFlowClusterHBHE.pfClusterBuilder.positionCalc.logWeightDenominatorByDetector[0].logWeightDenominator = hcal_thresholds[hcal_lumi]["rec"]
process.particleFlowClusterHBHE.pfClusterBuilder.allCellsPositionCalc.logWeightDenominatorByDetector[0].logWeightDenominator = hcal_thresholds[hcal_lumi]["rec"]
if hasattr(process,'particleFlowClusterHCAL'):
process.particleFlowClusterHCAL.pfClusterBuilder.allCellsPositionCalc.logWeightDenominatorByDetector[0].logWeightDenominator = hcal_thresholds[hcal_lumi]["rec"]
if hasattr(process,'particleFlowRecHitHBHE'):
process.particleFlowRecHitHBHE.producers[0].qualityTests[0].cuts[0].threshold = hcal_thresholds[hcal_lumi]["rec"]
for ctmod in ctmodules:
if hasattr(process,ctmod):
getattr(process,ctmod).HBThreshold1 = hcal_thresholds[hcal_lumi]["rec"][0]
getattr(process,ctmod).HBThreshold2 = hcal_thresholds[hcal_lumi]["rec"][1]
getattr(process,ctmod).HBThreshold = hcal_thresholds[hcal_lumi]["rec"][-1]

@mandrenguyen
Copy link
Contributor

Looking at the comparisons, they seem in line with the PR description
I was a bit surprised to see that the changes in the barrel are this large, for example:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_14_0_X_2023-11-19-0000+2e268c/59842/validateJR/14034.0_TTbar_14TeV+2023FS/all_RECO_step2/c_CaloTowersSorted_towerMaker__RECO_obj_obj_eta.png

@swagata87 Is this in line with your expectation?

@swagata87
Copy link
Contributor Author

@swagata87 Is this in line with your expectation?

I think this is what has happened-->
Noise cuts before and after this PR in HCAL depths (for 2023):
HB: 0.1, 0.2, 0.3, 0.3 --> 0.4, 0.3, 0.3, 0.3 [Changes in depth 1 and 2]
HE: 0.1, 0.2, 0.2, 0.2, 0.2, 0.2, 0.2 -> 0.1, 0.2, 0.2, 0.2, 0.2, 0.2, 0.2 [No change]
That's why we see changes only in barrel. I'd say this is expected.

@jfernan2
Copy link
Contributor

jfernan2 commented Dec 1, 2023

FYI @cms-sw/hcal-dpg-l2
Could you comment on this? Thanks

@abdoulline
Copy link

abdoulline commented Dec 1, 2023

I really appreciate Swagata did it and the changes seem to be fine, in line with CaloTowers cuts unification with PFlow ones (via PFCuts usage).

@jfernan2
Copy link
Contributor

jfernan2 commented Dec 4, 2023

+reconstruction

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 4, 2023

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

@antoniovilela
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.

7 participants