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

Replace buggy UL2016 EGM energy correction files #46046

Merged
merged 1 commit into from
Oct 3, 2024

Conversation

SanghyunKo
Copy link
Contributor

@SanghyunKo SanghyunKo commented Sep 18, 2024

PR description:

Replacing buggy UL2016 electron & photon energy correction data files, following the CMS talk thread and in preparation for the potential Run2 ReMiniAOD-ReNanoAOD campaign future (previously, the latest correction files only existed in the private repo - there was an official prescription to this for UL MiniAODv2, but it sneaked into the NanoAOD and became persistent since the newest data files were never merged to the official repo).

This PR is supposed to be work together with the data file update in cms-data/EgammaAnalysis-ElectronTools#12 (please see the discussion in this PR for more details).

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 18, 2024

cms-bot internal usage

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

  • PhysicsTools/NanoAOD (xpog)
  • RecoEgamma/EgammaTools (reconstruction)

@cmsbuild, @ftorrresd, @hqucms, @jfernan2, @mandrenguyen can you please review it and eventually sign? Thanks.
@AnnikaStein, @Prasant1993, @Sam-Harper, @a-kapoor, @afiqaize, @gpetruc, @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

@tvami
Copy link
Contributor

tvami commented Sep 18, 2024

@cmsbuild please test with cms-data/EgammaAnalysis-ElectronTools#12

@cmsbuild
Copy link
Contributor

+1

Size: This PR adds an extra 32KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-59e465/41605/summary.html
COMMIT: ea5e0e1
CMSSW: CMSSW_14_2_X_2024-09-18-1100/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/46046/41605/install.sh to create a dev area with all the needed externals and cmssw changes.

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:

You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-59e465/41605/git-recent-commits.json
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-59e465/41605/git-merge-result

Comparison Summary

Summary:

@SanghyunKo SanghyunKo marked this pull request as ready for review September 19, 2024 06:02
@jfernan2
Copy link
Contributor

+1

@hqucms
Copy link
Contributor

hqucms commented Sep 19, 2024

enable nano

@hqucms
Copy link
Contributor

hqucms commented Sep 19, 2024

please test with cms-data/EgammaAnalysis-ElectronTools#12

@RSalvatico
Copy link
Contributor

type egamma

@cmsbuild cmsbuild added the hold label Sep 19, 2024
@SanghyunKo
Copy link
Contributor Author

@hqucms After interacting with a previous convener and some studies, the issue was understood and presented in the PPD general [link]. In short, the difference in 2017/2018 is also expected from the change in the recommended prescription on how to apply energy scale correction systematics (not a bug in this case, though).

Hence, I think we can unhold this PR and cms-data/EgammaAnalysis-ElectronTools#12 and proceed.

@hqucms
Copy link
Contributor

hqucms commented Sep 26, 2024

@hqucms After interacting with a previous convener and some studies, the issue was understood and presented in the PPD general [link]. In short, the difference in 2017/2018 is also expected from the change in the recommended prescription on how to apply energy scale correction systematics (not a bug in this case, though).

Hence, I think we can unhold this PR and cms-data/EgammaAnalysis-ElectronTools#12 and proceed.

@SanghyunKo Regarding the changes on 2017 and 2018, I do see large diffs for the following files in cms-data/EgammaAnalysis-ElectronTools#12:

  • ScalesSmearings/Run2017_24Feb2020_runEtaR9Gain_v2_scales.dat
  • ScalesSmearings/Run2018_29Sep2020_RunFineEtaR9Gain_scales.dat

Do you confirm the changes are as expected? If so maybe you can update the title/description of cms-data/EgammaAnalysis-ElectronTools#12 to reflect that.

@SanghyunKo
Copy link
Contributor Author

Do you confirm the changes are as expected? If so maybe you can update the title/description of cms-data/EgammaAnalysis-ElectronTools#12 to reflect that.

Yes, that's what we observed from the comparison plots in the presentation. I updated the title/description in the data repository (left this one untouched as this is solely responsible for 2016).

@hqucms
Copy link
Contributor

hqucms commented Sep 26, 2024

unhold

@cmsbuild cmsbuild removed the hold label Sep 26, 2024
@hqucms
Copy link
Contributor

hqucms commented Sep 26, 2024

+1

@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, @rappoccio, @sextonkennedy, @mandrenguyen (and backports should be raised in the release meeting by the corresponding L2)
Notice This PR was tested with additional Pull Request(s), please also merge them if necessary: cms-data/EgammaAnalysis-ElectronTools#12

@hqucms
Copy link
Contributor

hqucms commented Sep 26, 2024

@SanghyunKo Should this be backported to 10_6_X?

@SanghyunKo
Copy link
Contributor Author

@SanghyunKo Should this be backported to 10_6_X?

We are collecting surveys to know whether there are some analyzers who need to re-nano their data with 10_6_X urgently. If we have such a request, then we need a backport. Otherwise, it's not necessary if everybody agrees to wait for nano v15.

But regardless of that, I think that doing backport is no harm and I see no reason to keep this bug persistent. So personally, I'd prefer to fix this issue once and for all unless there is a good reason not to do the backport.

@hqucms
Copy link
Contributor

hqucms commented Sep 27, 2024

@SanghyunKo Should this be backported to 10_6_X?

We are collecting surveys to know whether there are some analyzers who need to re-nano their data with 10_6_X urgently. If we have such a request, then we need a backport. Otherwise, it's not necessary if everybody agrees to wait for nano v15.

But regardless of that, I think that doing backport is no harm and I see no reason to keep this bug persistent. So personally, I'd prefer to fix this issue once and for all unless there is a good reason not to do the backport.

Sounds good. Please make a backport.

@SanghyunKo
Copy link
Contributor Author

Backport PR opened #46138

@antoniovilela
Copy link
Contributor

cms-data/EgammaAnalysis-ElectronTools#12 is merged
cmsdist PR: cms-sw/cmsdist#9448

@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