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

[Urgent] Update CPX tune to 13.6 TeV #38358

Merged
merged 3 commits into from
Jun 15, 2022

Conversation

SanghyunKo
Copy link
Contributor

PR description:

Update CPX tune to 13.6 TeV, see https://indico.cern.ch/event/1169048/#30-sample-normalization-for-cp

PR validation:

if this PR is a backport please specify the original PR and why you need to backport that PR:

This PR is meant to be backported to 12_4_X & 12_2_X for Run 3 MC production

@SanghyunKo
Copy link
Contributor Author

@qliphy
Copy link
Contributor

qliphy commented Jun 13, 2022

urgent

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38358/30548

  • This PR adds an extra 28KB to repository

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @SanghyunKo (Sanghyun Ko) for master.

It involves the following packages:

  • Configuration/Generator (generators)

@SiewYan, @mkirsano, @Saptaparna, @cmsbuild, @GurpreetSinghChahal, @alberto-sanchez can you please review it and eventually sign? Thanks.
@Martin-Grunewald, @missirol, @fabiocos this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

@qliphy
Copy link
Contributor

qliphy commented Jun 13, 2022

please test

@Saptaparna
Copy link
Contributor

Hi @SanghyunKo, based on the discussion with Sercan, it could be useful to have 2 sets of settings one for 13TeV and the other for 13.6 TeV in the event that someone uses this campaign for Run II

@SanghyunKo
Copy link
Contributor Author

@Saptaparna I was thinking about this, but I guess one should use 10_6 for Run 2. Of course we can generate either of 13/13.6 TeV samples with a single release in the GEN point of view, however consecutive SIM-DIGI-RECO step is entirely specialized for the specific run for each release, no?

@Saptaparna
Copy link
Contributor

Indeed, actually now that I think about it a bit more, we have a genchecking script level check to make sure that the c.o.m is 13.6 TeV. So, this should be fine. +1

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-08e0d6/25492/summary.html
COMMIT: d6adb93
CMSSW: CMSSW_12_5_X_2022-06-13-1100/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/38358/25492/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: 45833 differences found in the comparisons
  • DQMHistoTests: Total files compared: 50
  • DQMHistoTests: Total histograms compared: 3658882
  • DQMHistoTests: Total failures: 428037
  • DQMHistoTests: Total nulls: 412
  • DQMHistoTests: Total successes: 3230411
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 2759.392 KiB( 49 files compared)
  • DQMHistoSizes: changed ( 11634.0,... ): 460.075 KiB HLT/HLTEgammaValidation
  • DQMHistoSizes: changed ( 11834.0 ): -1.055 KiB SiStrip/MechanicalView
  • DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
  • Checked 208 log files, 45 edm output root files, 50 DQM output files
  • TriggerResults: found differences in 10 / 49 workflows

@Saptaparna
Copy link
Contributor

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

@qliphy
Copy link
Contributor

qliphy commented Jun 14, 2022

@SanghyunKo @Saptaparna Surely for Run3 production only these updated CPX tune files will be used. However, please check the comparison differences are as expected:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_12_5_X_2022-06-13-1100+08e0d6/50968/validateJR.html

@qliphy
Copy link
Contributor

qliphy commented Jun 15, 2022

please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-08e0d6/25528/summary.html
COMMIT: 2bd5f35
CMSSW: CMSSW_12_5_X_2022-06-14-2300/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/38358/25528/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: 6 differences found in the comparisons
  • DQMHistoTests: Total files compared: 50
  • DQMHistoTests: Total histograms compared: 3659074
  • DQMHistoTests: Total failures: 14
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3659038
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 49 files compared)
  • Checked 208 log files, 45 edm output root files, 50 DQM output files
  • TriggerResults: no differences found

@qliphy
Copy link
Contributor

qliphy commented Jun 15, 2022

@cms-sw/generators-l2 Please consider signing this PR thus we can include the backported one in 12_4_0.

@Saptaparna
Copy link
Contributor

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

@perrotta
Copy link
Contributor

+1
-Physics validation of the new tunes in https://indico.cern.ch/event/1169048/#30-sample-normalization-for-cp

@agrohsje
Copy link

Dear @cms-sw/generators-l2, @SanghyunKo
there is small "steering" problem with this PR for our automatic background production. The files are named e.g. PythiaCP1Settings13p6TeV_cfi.py, so CP1 and 13p6TeV are pulled apart and split by Settings, similar inside the file we have, e.g.:
pythia8CP1Settings13p6TeVBlock and pythia8CP1Settings13p6TeV.
In the first case: Settings and Block are even pulled apart, although they belong together.
The handling is a bit ugly as we would need to have two parameters to get the name right. To simplify, can we just leave the old names and call the folder MCTunes_Run3_13p6TeV or something like this? That would simplify steering a lot. Also for users who can easily update from UL fragments by just changing one line instead of having all the time 13p6TeV to add somewhere.
Cheers, Alexander.

@SanghyunKo
Copy link
Contributor Author

@agrohsje seems like it's going back to the initial commit but with a separate folder. Is it okay-ish to make two different PSet coexist with the same name? (which why I put 13p6TeV) Otherwise, I don't have any strong opinion on the naming convention frankly - I volunteered for this PR simply because no one else could do this before the 12_4_0 deadline. Unfortunately, 12_4_0 is already there and I don't have any good picture of the next release & MC production schedule (or how the new automatized steering handles the PSet name). So maybe it'd be more efficient to discuss with someone who has a better picture than me. Perhaps L2 knows whether @agrohsje still has a chance for another PR? @Saptaparna @GurpreetSinghChahal @menglu21

@Saptaparna
Copy link
Contributor

I agree with @SanghyunKo. Thanks to @qliphy for helping us with the inclusion of this PR into 12_4. @agrohsje will it be possible for you to read these files in their current format or is that a no-go?

@agrohsje
Copy link

A separate folder with a proper specification of beam energy should fulfill all requirements, no? Is there a reason to include 13p6TeV in so many different places: the file names, the options inside the files. If we just have it in one place that would be very convenient. I think also for users the new files are not ideal/handy. @qliphy @perrotta would you be ok, with removing 13p6TeV everywhere and just add it to the folder name e.g. MCTunes_Run3_13p6TeV.
The current structure requires unnecessary hardcoding in the common background code. Of course we can mimic everything, but I think if we can fix now and ORP agrees that looks like the best solution to me.

@agrohsje
Copy link

And let me add. My comment was not against what was done. I know that Sanghyun just stepped up and helped out. So that was really great. I just realized that the current solution is not ideal from a generic computing perspective.

@sihyunjeon
Copy link
Contributor

Just throwing random stuffs, I saw PPD slides and looks like 12_4_1 is going to be used in the end, and do we still have time for that?

@qliphy
Copy link
Contributor

qliphy commented Jun 24, 2022

@agrohsje @SanghyunKo @cms-sw/generators-l2 12_4_1 will be cut by next Tuesday (June 28th). So please make relevant PRs the sooner the better to include the comments as mentioned above by @agrohsje if agreed by everyone:
"To simplify, can we just leave the old names and call the folder MCTunes_Run3_13p6TeV or something like this? That would simplify steering a lot. Also for users who can easily update from UL fragments by just changing one line instead of having all the time 13p6TeV to add somewhere."

@sihyunjeon
Copy link
Contributor

@qliphy Thanks for the confirmation!

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.

8 participants