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

Use uncorrected H/E for electron energy regressions #45470

Merged
merged 1 commit into from
Jul 16, 2024

Conversation

Sam-Harper
Copy link
Contributor

PR description:

This is a bug fix PR which we also want to back port to 14_0_X

The electron energy regressions are erroneously using the energy regression corrected H/E and this PR fixes this.

There are two types of H/E in E/gamma, cone based (hcalOverEcal) and single tower behind cluster (hcalOverEcalBc). Historically the hcalOverEcal is also adjusted when the corrected energy of the electron is adjusted, ie the E is the best estimate of the electron ecal only energy.

When the H/E was being overhauled, hcalOverEcalBc was harmonised with this behaviour, it also got the energy correction

current:
https://github.com/cms-sw/cmssw/blob/CMSSW_14_0_11/DataFormats/EgammaCandidates/src/GsfElectron.cc#L176-L190
before:
https://github.com/cms-sw/cmssw/blob/CMSSW_11_0_0/DataFormats/EgammaCandidates/src/GsfElectron.cc#L174-L184

I can see why this was seen as reasonable at the time but it has a nasty consequence which is that the hcalOverEcalBc variable is used in the energy regression. So when you go to apply the regression, you see the H/E value adjusted with the currently applied regression.

This is bad as now its very difficult to reapply the regression. Because the H/E value includes the last regression. Also it makes the training bad because it has the wrong variable.

in the training the H/E is after the current releases regression. But when you apply it in the release its based on the intermediate ecal regression which is completely unmaintained and as far as I'm aware has little to no effect (although possibly has some weak PF implications) as it very quickly gets overriden by the "main" energy regression. So the variable the energy regression is trained on and the variable it is applied on is different. And then you reapply it, you get a different answer again as the H/E already has the regression applied to it.

The UL Run2 regressions were trained on the uncorrected H/E variable and thus they are also getting wrong input variable. The 2022 Run3 regression was trained with the corrected H/E but a different correction to the one its seeing when its applied. The 2024 which are being developed should hopefully be with the uncorrected value.

We have the uncorrected H/E variable in the form of the full5x5_hcalOverEcal and full5x5_hcalOverEcalBc variables which are identical (due to how the showershape block works) but do not get the the correction. This PR moves it to that.

While this is not the variable the 2022 regression was trained with, neither is the variable currently being applied. While it will fix the Run2 ones to their correct value. I hope this can be back ported to 14_0_X as it is a bug fix.

Note I left the ability to emulate the bug in if people really want but I dont think thats so necessary in my opinon.

PR validation:

Tested locally and it gives the expected results. On going for the physics impact. For electrons it should mostly be the same and should have little change. Jets/badly measured electrons will have likely larger changes but expect it to more or less changes from one randomish value to another so again limited physics impact just the values will change.

Run the matrix i was getting a lot of DAS errors locally so easier just to test here.

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 16, 2024

cms-bot internal usage

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @Sam-Harper for master.

It involves the following packages:

  • RecoEgamma/EgammaTools (reconstruction)

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

cms-bot commands are listed here

@Sam-Harper
Copy link
Contributor Author

please test

@RSalvatico
Copy link
Contributor

type egamma

@cmsbuild
Copy link
Contributor

+1

Size: This PR adds an extra 12KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-573dc8/40430/summary.html
COMMIT: fd2862a
CMSSW: CMSSW_14_1_X_2024-07-16-1100/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/45470/40430/install.sh to create a dev area with all the needed externals and cmssw changes.

  • DAS Queries: The DAS query tests failed, see the summary page for details.

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 4291 differences found in the comparisons
  • DQMHistoTests: Total files compared: 48
  • DQMHistoTests: Total histograms compared: 3345094
  • DQMHistoTests: Total failures: 8656
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3336418
  • DQMHistoTests: Total skipped: 20
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 47 files compared)
  • Checked 202 log files, 165 edm output root files, 48 DQM output files
  • TriggerResults: no differences found

@mandrenguyen
Copy link
Contributor

type bug-fix

@mandrenguyen
Copy link
Contributor

+1
Should directly merge, assuming my orp rights are active now.
On the reco side, comparisons show expected effects: small changes to egamma related distribution.
Regarding a possible backport, we would have to discuss with PPD whether it's worth modifying the reco in the middle of prompt production.

@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 be automatically merged.

@cmsbuild cmsbuild merged commit b08e6a8 into cms-sw:master Jul 16, 2024
11 checks passed
@RSalvatico
Copy link
Contributor

+1 Should directly merge, assuming my orp rights are active now. On the reco side, comparisons show expected effects: small changes to egamma related distribution. Regarding a possible backport, we would have to discuss with PPD whether it's worth modifying the reco in the middle of prompt production.

Thanks! @cms-sw/ppd-l2 letting PPD know (I have informed the coordinators about this today)

@malbouis
Copy link
Contributor

Thanks @mandrenguyen, @RSalvatico and @Sam-Harper . Indeed, considering this a bug fix, we would like to have it in a 14_0_X release for the upcoming 2024 MC production and 2024 partial ReRECO (and Prompt).

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