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

Update pileup scenario in RelVals #36388

Closed
wants to merge 3 commits into from
Closed

Conversation

kskovpen
Copy link
Contributor

@kskovpen kskovpen commented Dec 7, 2021

PR description:

This PR updates the pileup scenario in RelVals from the default Run3_Flat55To75_PoissonOOTPU to a more realistic 2022_LHC_Simulation_10h_2h, which was already validated in 12_0_0_pre6.

PR validation:

Tested in standard PU wfs.

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

Not a backport.

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 7, 2021

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36388/27178

  • This PR adds an extra 48KB to repository

  • There are other open Pull requests which might conflict with changes you have proposed:

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 7, 2021

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

It involves the following packages:

  • Configuration/PyReleaseValidation (pdmv, upgrade)

@jordan-martins, @bbilin, @wajidalikhan, @cmsbuild, @AdrianoDee, @srimanob, @kskovpen can you please review it and eventually sign? Thanks.
@makortel, @kpedro88, @Martin-Grunewald, @missirol, @fabiocos, @slomeo 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

@kskovpen
Copy link
Contributor Author

kskovpen commented Dec 7, 2021

please test

@srimanob
Copy link
Contributor

srimanob commented Dec 7, 2021

Hi @kskovpen

I am not sure it is a good idea to change to the PU scenario used for production in relvals setting. The reason is that we have very limited no. of events in relvals, getting very broad PU will not help us to focus on PU dependent quantities. In the past, i.e. 2018, we decided to keep PU50 for relvals, just to make sure we get relvals in the worst-case scenario.

Maybe @cms-sw/reconstruction-l2 and DPG @amassiro should comment on this PR for the usefulness for objects and physics performance.

@kskovpen
Copy link
Contributor Author

kskovpen commented Dec 7, 2021

Hi @kskovpen

I am not sure it is a good idea to change to the PU scenario used for production in relvals setting. The reason is that we have very limited no. of events in relvals, getting very broad PU will not help us to focus on PU dependent quantities. In the past, i.e. 2018, we decided to keep PU50 for relvals, just to make sure we get relvals in the worst-case scenario.

Maybe @cms-sw/reconstruction-l2 and DPG @amassiro should comment on this PR for the usefulness for objects and physics performance.

Thanks for your comment, @srimanob. The only goal was to update to the same pileup configuration as used in the MC production, but I get your point. Let's see indeed what others think.

@mmusich
Copy link
Contributor

mmusich commented Dec 7, 2021

Hi @srimanob reporting here my 2 cents:

am not sure it is a good idea to change to the PU scenario used for production in relvals setting. The reason is that we have very limited no. of events in relvals, getting very broad PU will not help us to focus on PU dependent quantities. In the past, i.e. 2018, we decided to keep PU50 for relvals, just to make sure we get relvals in the worst-case scenario.

  1. Run3_Flat55To75_PoissonOOTPUhas no fixed average as in PU50, but varies from 55 to 75 so it already somewhat spread, isn't it?
  2. I think the current profile is overly aggressive w.r.t LHC projections and forces upon developers unrealistically pessimistic conditions (e.g. dynamic inefficiencies depend on PU etc.). This has already had some consequences e.g. on the tuning of tracking for HLT.

@slava77
Copy link
Contributor

slava77 commented Dec 7, 2021

a link to the PU profile plot in this PR description would be nice.

@mmusich
Copy link
Contributor

mmusich commented Dec 7, 2021

a link to the PU profile plot in this PR description would be nice.

I think it was discussed here: #34460 (comment)

@srimanob
Copy link
Contributor

srimanob commented Dec 7, 2021

Hi @srimanob reporting here my 2 cents:

am not sure it is a good idea to change to the PU scenario used for production in relvals setting. The reason is that we have very limited no. of events in relvals, getting very broad PU will not help us to focus on PU dependent quantities. In the past, i.e. 2018, we decided to keep PU50 for relvals, just to make sure we get relvals in the worst-case scenario.

  1. Run3_Flat55To75_PoissonOOTPUhas no fixed average as in PU50, but varies from 55 to 75 so it already somewhat spread, isn't it?
  2. I think the current profile is overly aggressive w.r.t LHC projections and forces upon developers unrealistically pessimistic conditions (e.g. dynamic inefficiencies depend on PU etc.). This has already had some consequences e.g. on the tuning of tracking for HLT.

Hi @mmusich
Thanks for the comments. I don't want to close the option, a new (prod) one or third option is fine. I just want us to have the discussion before changing for usefulness of relvals.

@amassiro
Copy link
Contributor

amassiro commented Dec 7, 2021

Hi @kskovpen
I am not sure it is a good idea to change to the PU scenario used for production in relvals setting. The reason is that we have very limited no. of events in relvals, getting very broad PU will not help us to focus on PU dependent quantities. In the past, i.e. 2018, we decided to keep PU50 for relvals, just to make sure we get relvals in the worst-case scenario.
Maybe @cms-sw/reconstruction-l2 and DPG @amassiro should comment on this PR for the usefulness for objects and physics performance.

Thanks for your comment, @srimanob. The only goal was to update to the same pileup configuration as used in the MC production, but I get your point. Let's see indeed what others think.

No strong opinion here.
PU50 has already the poissonian smearing around 50, then it could be enough. However, keep as a reference "up to 60 pu" as possible scenarios for run 3. Tomorrow at the Run Coordination report during the CMS week you will see more details about luminosity scenarios for 2022, and full Run 3.

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 7, 2021

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-eb3b33/21048/summary.html
COMMIT: 7f47da4
CMSSW: CMSSW_12_3_X_2021-12-06-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/36388/21048/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-eb3b33/21048/git-recent-commits.json
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-eb3b33/21048/git-merge-result

Comparison Summary

Summary:

  • No significant changes to the logs found
  • ROOTFileChecks: Some differences in event products or their sizes found
  • Reco comparison results: 10245 differences found in the comparisons
  • DQMHistoTests: Total files compared: 42
  • DQMHistoTests: Total histograms compared: 3250601
  • DQMHistoTests: Total failures: 10471
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3240108
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 41 files compared)
  • Checked 177 log files, 37 edm output root files, 42 DQM output files
  • TriggerResults: no differences found

@srimanob
Copy link
Contributor

Kindly ping the discussion on this @qliphy @perrotta

@srimanob
Copy link
Contributor

Do you have a plan to discuss this PR, @cms-sw/pdmv-l2 ?

@kskovpen
Copy link
Contributor Author

It was briefly discussed yesterday. I think there is no strong opinion on this. On our side, the idea was just to keep things in sync with respect to what is used in MC production. Since RelVals are mainly used for software validation, probably this update is not very relevant, having both pileup profiles more or less representing similar phase space.

@kskovpen
Copy link
Contributor Author

kskovpen commented Feb 1, 2022

As was discussed today, let's close it for now.

@kskovpen kskovpen closed this Feb 1, 2022
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.

None yet

6 participants