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

Apply HCal noise thresholds to HCal detector-based isolation of electrons #43355

Merged
merged 2 commits into from
Nov 23, 2023

Conversation

swagata87
Copy link
Contributor

PR description:

It looks like HCal noise thresholds are not used in HCal detector-based isolation of electrons. Fixing that in this PR. This isolation is used in HEEP ID. There could be other use-cases also. This bug/feature affects 2022 and 2023. Before that, this variable was built from caloTowers, where HCal thresholds are generally applied. At the beginning of Run3, EGamma dropped the usage of caloTower, and HCal detector-based isolation variable was built instead from HCal recHits (#33666). It's not clear why at that point it was decided to not use the HCal noise thresholds, most probably it's an oversight. In any case, given the HCal noise issues in Run3, which caused severe data/MC disagreement, there is no good reason to not apply the noise cuts which at least helps to some extent to match data and MC better.

It was understood in the context of #43164. Most of the reco differences observed there (#43164 (comment)) are likely due to this bug/feature.

PR validation:

Tested with a Run3 workflow.

@cmsbuild
Copy link
Contributor

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-43355/37822

Code check has found code style and quality issues which could be resolved by applying following patch(s)

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-43355/37823

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

  • RecoEgamma/EgammaElectronAlgos (reconstruction)

@jfernan2, @cmsbuild, @mandrenguyen can you please review it and eventually sign? Thanks.
@ram1123, @Sam-Harper, @lgray, @varuns23, @afiqaize, @sobhatta, @a-kapoor, @jainshilpi, @valsdav, @Prasant1993, @missirol, @sameasy this is something you requested to watch as well.
@sextonkennedy, @rappoccio, @antoniovilela you are the release manager for this.

cms-bot commands are listed here

@mandrenguyen
Copy link
Contributor

please test

@swagata87
Copy link
Contributor Author

type egamma

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-f01f5f/35995/summary.html
COMMIT: 8eb17c4
CMSSW: CMSSW_14_0_X_2023-11-21-2300/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/43355/35995/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

@mandrenguyen
Copy link
Contributor

+reconstruction
Will this be backported?

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

@antoniovilela
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 3d38a1d into cms-sw:master Nov 23, 2023
11 checks passed
@swagata87
Copy link
Contributor Author

Will this be backported?

In general, a backport might not be very necessary, as the variable in question (HCAL detector-based isolation) is not widely used by POGs or PAGs. Most people use HCAL PF cluster based isolation, or PF-candidate based isolation, which are better variables.

If EGM POG wants (since HEEP ID use it), and if there is a re-reco of 2022 and 2023 data in a release older than where this is merged, then a backport can be made. Tagging EGamma convenors @cms-sw/egamma-pog-l2

@swagata87
Copy link
Contributor Author

Just confirmed from PPD that the rereco of 2022+2023 will be with 14_0_X, where this PR is merged. So I think no backport is needed.

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.

4 participants