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

dynamic conditions for PPS direct simulation #32207

Merged
merged 30 commits into from Dec 8, 2020

Conversation

jan-kaspar
Copy link
Contributor

PR description:

This PR improves the PPS direct simulation by allowing for non-constant conditions during a single cmsRun execution. This is essential for PPS since the relevant conditions (often LHC related) change during each year (often at LHC Technical Stop, "TS").

The proposed solution was presented in Proton POG meeting. In short, there's a new ES setup module which randomly generates conditions relevant for PPS simulation (at configurable frequency). The random generation follows distributions extracted from data, in proportions following the integrated luminosity.

PR validation:

The two PDF summaries below compare the results obtained with SW before (blue) and after (red dashed) this PR.

  • reco_cmp.pdf - no difference in data reconstruction (as expected)
  • dirsim_cmp.pdf - some differences in direct simulation (as expected)
    • before this PR, the simulation could be performed only for a single period, thus the "per year" rows miss the blue curve
    • row "2016", the two periods (pre- and post-TS2) are correctly represented in the xangle histogram (185 an 140 urad, resp) and with the correct weight (higher L_int in pre-TS2)
    • row "2016 postTS2" - perfect agreement before vs. after as expected
    • row "2017": both beta* values (0.4 for pre-TS2 and 0.3 for post-TS2) are correctly represented
    • rows "2017_postTS2" and "2018_postTS2" - compatible shapes but not perfect overlap - expected since this PR changes the order or random values generations, thus the samples of xangles/beta* are a little different

@smuzaffar smuzaffar modified the milestones: CMSSW_11_2_X, CMSSW_11_3_X Nov 26, 2020
@jan-kaspar
Copy link
Contributor Author

@cms-sw/alca-l2 Can we facilitate your review in any way?

@silviodonato
Copy link
Contributor

urgent

@cmsbuild cmsbuild added the urgent label Dec 1, 2020
@silviodonato
Copy link
Contributor

@cms-sw/alca-l2 ?

@silviodonato
Copy link
Contributor

@jan-kaspar I tried this PR with

cmsDriver.py stepTest  --conditions auto:phase1_2021_realistic -s RAW2DIGI,L1Reco,RECO --datatier GEN-SIM-RECO -n 10 --geometry DB:Extended --era Run3 --eventcontent RECOSIM --filein file:/afs/cern.ch/work/s/sdonato/public/debug_PPS/badd237c-4857-4f48-b10f-52c832f57f02_ev1126.root

(see #32340).

And I'm still getting

----- Begin Fatal Exception 01-Dec-2020 11:51:52 CET-----------------------
An exception of category 'NoRecord' occurred while
   [0] Processing  Event run: 1 lumi: 12 event: 1126 stream: 0
   [1] Running path 'RECOSIMoutput_step'
   [2] Prefetching for module PoolOutputModule/'RECOSIMoutput'
   [3] Prefetching for module LogErrorHarvester/'logErrorHarvester'
   [4] Calling method for module CTPPSProtonProducer/'ctppsProtons'
Exception Message:
No "LHCInfoRcd" record found in the EventSetup.n
 Please add an ESSource or ESProducer that delivers such a record.
----- End Fatal Exception -------------------------------------------------

Please note that in #32340 we are getting error in Run-3 and HI workflows.

@jan-kaspar
Copy link
Contributor Author

My comment #32340 (comment) was more a conceptual one. I apologise if it was understood that this PR would fix the problem.

This PR applies the idea of "dynamically generated" conditions to the "direct" simulation of PPS (https://twiki.cern.ch/twiki/bin/viewauth/CMS/TaggedProtonsDirectSimulation) not to the standard simulation. But I guess that the same concept can be used also for the standard simu (TBC).

In general, PPS is still missing several pieces for 2021 scenario, let me just mention this PR #31484.

Therefore I guess that it is appropriate to (temporarily) disable PPS reco (and possibly simu) for Run3 wfs - as I saw that you prepare elsewhere.

@civanch
Copy link
Contributor

civanch commented Dec 1, 2020

@jan-kaspar , if PPS would be disabled from SIM for Run-3 WFs, then it will be never enabled back for the Run-3 standard production, because the production geometry will be frozen. If there is no geometry, we have no SIM hits, DIGI-RECO will be configured assuming no PPS hits. All analysis will remain private.

This may be a reality but why not to try to push PPS to be on some acceptable level within CMSSW? At least, to have correct geometry description.

@jan-kaspar
Copy link
Contributor Author

@civanch Thanks for pointing out technical details for simu - I acknowledge that I am not an expert. I wish to comment only for reco. I dared to mention simu in my previous comment since I am puzzled. It seems that the LHCInfo is missing for the WF. This means that, e.g. the LHC xangle is undefined. If it is undefined, then one cannot tell how to propagate forward protons from the IP to the RPs. Consequently I don't know what the simu does...

As for the solution of the current "crisis" I'd agree to temporarily disable PPS reco. We should re-enable it once the problem with conditions is understood and solved.

@jan-kaspar
Copy link
Contributor Author

For the recent dicussions in #32340 (comment) and elsewhere, let me comment that:

  • this PR adds the possibility to use the "dynamic" conditions (not an obligation)
  • this PR only touches the "direct" (fast) simulation of PPS, thus no central workflow
  • for PPS use, this PR has been approved by the Proton POG
    Therefore I believe that it is still worth merging it, while we can have more general discussion meanwhile.

@silviodonato
Copy link
Contributor

@cms-sw/alca-l2 please review/sign this PR. My comment was unrelated to this PR (see #32346 ).

@silviodonato
Copy link
Contributor

urgent
if there are no comments/objections, I will merge it tomorrow after the meeting

@christopheralanwest
Copy link
Contributor

+alca

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 7, 2020

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

@silviodonato
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit b4e0fd7 into cms-sw:master Dec 8, 2020
@jan-kaspar jan-kaspar deleted the dirsim_dynamic_conditions branch August 31, 2021 08:28
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

9 participants