-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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 pdigi_hi_nogen for HI MC wfs #35028
Conversation
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35028/24886
|
A new Pull Request was created by @mandrenguyen (Matthew Nguyen) for master. It involves the following packages:
@jordan-martins, @chayanit, @bbilin, @wajidalikhan, @kpedro88, @cmsbuild, @AdrianoDee, @srimanob, @kskovpen can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
test parameters:
|
please test |
test parameters:
|
@cmsbuild please test |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-5de25b/18057/summary.html Comparison Summary@slava77 comparisons for the following workflows were not done due to missing matrix map:
Summary:
|
@mandrenguyen Could you please go through the report, Thanks. |
@srimanob I checked that wf 312 runs at the prompt without issue. I looked thru the log files, but I'm not spotting anything relevant. Are you able to see where the code is crashing? |
Hi @mandrenguyen |
@srimanob Ah, so it's not a crash. The |
There are 2 possibilities here. Since DIGI:pdigi_hi_nogen is available since 2018, #24199, we can either
Since the effect on this PR is very limited to HI GEN MC, I would prefer to merge it, and try to run at the next pre-release. Do you agree @qliphy @perrotta @cms-sw/pdmv-l2 ? |
+Upgrade |
please test |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-5de25b/20525/summary.html Comparison Summary@slava77 comparisons for the following workflows were not done due to missing matrix map:
Summary:
|
+1 |
+1 |
Looking at the comparison plots there are quite a lot of differences, and I assume they are kind of expected. |
@perrotta This should only have changed gen quantities. I'm a bit at a loss as to how we can have changes to reco quantities. |
Uhm, since you touched the digi step I assumed that it would have forcibly propagated to reco as well... |
Ah, I have updated the release for the pileup dataset. So that's where this effect comes from. |
Indeed, that was my understanding |
Yes, I would say so. Sorry for the noise. |
+operations |
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) |
+1 |
PR description:
This PR switches to the DIGI:pdigi_hi_nogen for heavy-ion MC RelVal workflows.
Back in #24199 the option DIGI:pdigi_hi_nogen was introduced.
The previously used pdigi_hi option adds some heavy-ion generator level quantities, such as npart and ncoll, to the output.
The 'nogen' extension prevents gen collections from being reproduced in the digi step.
Most heavy-ion MC requests use an event overlay workflow, with a crossing frame to mix the gen info from the signal and background events.
If the gen info is reproduced at the digi step, we lose the gen info from the background event.
The pdigi_hi_nogen has long been the default option in official MC, but we neglected to switch to this option in the RelVal workflows, which occasionally leads to confusion.
This PR fixes that.
2nd commit: A new validation sequence for the heavy-ion event overlay workflow is added. It ignores genParticles that are not from collisionID = 0 (the signal event)
PR validation:
Tested on wf 159 and 301
if this PR is a backport please specify the original PR and why you need to backport that PR:
No backport needed