Add GEN/LHE weight validation subdirectories for GEN Relval #36994
Conversation
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36994/28387 |
|
A new Pull Request was created by @SanghyunKo (Sanghyun Ko) for master. It involves the following packages:
@SiewYan, @mkirsano, @emanueleusai, @ahmad3213, @cmsbuild, @GurpreetSinghChahal, @jfernan2, @Saptaparna, @alberto-sanchez, @pmandrik, @pbo0, @rvenditti can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
@SanghyunKo please add yourself along with your github username in comments in the DQM GEN Validators e-group: |
|
please test workflow 504, 555, 556 |
|
@jfernan2 Thanks for letting me know, I've added myself to the e-group. |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-2f4d7b/22493/summary.html Comparison SummaryThe workflows 1001.0, 1000.0, 136.88811, 136.874, 136.8311, 136.793, 136.7611, 136.731, 4.22 have different files in step1_dasquery.log than the ones found in the baseline. You may want to check and retrigger the tests if necessary. You can check it in the "files" directory in the results of the comparisons @slava77 comparisons for the following workflows were not done due to missing matrix map:
Summary:
|
|
@SanghyunKo I understand all the histograms added: I wonder if you could add some switch in the code to only produce them when the WF is based on an external LHE generator |
|
Pull request #36994 was updated. @SiewYan, @mkirsano, @emanueleusai, @ahmad3213, @cmsbuild, @GurpreetSinghChahal, @jfernan2, @Saptaparna, @alberto-sanchez, @pmandrik, @pbo0, @rvenditti can you please check and sign again. |
|
please test workflow 504, 555, 556 |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-2f4d7b/22898/summary.html Comparison Summary@slava77 comparisons for the following workflows were not done due to missing matrix map:
Summary:
|
|
+1 |
|
+1 |
cmsbuild
added
fully-signed
generators-approved
and removed
generators-pending
pending-signatures
labels
|
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 |
There was a problem hiding this comment.
@SanghyunKo I was investigating the possible origin of the comparison errors that show up for wf 135.4 in the recent PR tests for the histogram made by this wgtVal_, which suggests a non reproducibility issue possibly due to some non initialized values.
In this line of the code I found a possible culprit: if I am not wrong this should have been normalized to the first weight in the vector, i.e. index 0, while if you normalize to the second element in the vector (index 1) you can get some undefinite result in case the number of elements in the vector is lower than two,
Could you please check at your earliest, and either apply this fix (if you think it is correct), or find and then implement the appropriate one? Thank you.
| nlogWgt_->Fill(std::log10(weights_.at(idxGenEvtInfo_).size()), weight_); | ||
|
|
||
| for (unsigned idx = 0; idx < weights_.at(idxGenEvtInfo_).size(); idx++) | ||
| wgtVal_->Fill(weights_.at(idxGenEvtInfo_)[idx] / weights_.at(idxGenEvtInfo_)[1], weight_); |
There was a problem hiding this comment.
Shouldn't this be
wgtVal_->Fill(weights_.at(idxGenEvtInfo_)[idx] / weights_.at(idxGenEvtInfo_)[0], weight_);instead?
There was a problem hiding this comment.
@perrotta this was intended on purpose since the proper normalization of PS weight is done by dividing weights to the baseline weight, which is located at idx 1 (Twiki). But indeed I can place a protector for the case weights_.at(idxGenEvtInfo_).size()=1 between other protectors for size()=0 case and size()<=idxMax_ case
(filling weights_.at(idxGenEvtInfo_)[0]/weights_.at(idxGenEvtInfo_)[0] for size()=1 case is redundant)
if (weights_.at(idxGenEvtInfo_).size()<2)
return; // no baseline weight in GenEventInfo
for (unsigned idx = 0; idx < weights_.at(idxGenEvtInfo_).size(); idx++)
wgtVal_->Fill(weights_.at(idxGenEvtInfo_)[idx] / weights_.at(idxGenEvtInfo_)[1], weight_);
|
Thank you @SanghyunKo Would it be possible that there is not such baseline weight, only the nominal one (i.e. weights_.at(idxGenEvtInfo_).size() = 1)? Did you check whether this was really the origin of the supposed non-sensical entries in the I think we can submit a fix PR with your suggestion, which is a reasonable one even if it does not fix the issue seen in the PR comparisons. But it wouldn't be bad if we could make some simple check to verify that it actually fixes it. |
|
@perrotta would you mind providing some snippet or link to the failing comparison you mentioned? It's not clear to me what you're referring to... If I get it then I can definitely run a quick test for it. As for the number of weights, there should be both nominal & baseline weight when we have PS weights, but I realized that this isn't always true when we talk about Relvals (though it is mostly true in official samples). |
|
@SanghyunKo For the failing comparison, you can check several recent PR tests: or |
|
The ASAN build is reporting a out-of-bounds memory read coming from |
|
Thank you @Dr15Jones : your observation is perfectly in line with what was discussed earlier on in this thread. Since @SanghyunKo already proposed a fix, but such a fix was not submitted yet, I shamelessly copied the very same solution proposed by @SanghyunKo in a new PR, #37185, in order to speed up the possible integration in CMSSW in time for 12_3_0_pre6. Of course, if @SanghyunKo or anyone else finds a more appropriate solution, that PR can be closed and we can move to the new one. |
|
Thanks @perrotta and no problem at all, it's my bad. I was struggling to reproduce the failing DQM comparison (but I couldn't) but the address sanitizer is telling us the fix is needed anyway indeed. |
PR description:
Adding Relval subdirectories
GenWeight&LHEWeightto detect odds in GEN/LHE weight contents, to avoid any potential issues that we had experienced before - such as #36705, #27918, cms-sw/cmsdist#6688, hypernews, and many other...Each GEN/LHE subdirectory contains the number of weights, distribution of weights (normalized to the nominal), leading lepton/jet Pt & η. In addition, GEN directory contains ISR/FSR up/down (2 or 1/2) variations and their ratio to the nominal, while LHE directory contains envelop of scale variations and PDF uncertainty (± RMS) and their ratio to the nominal. Assumed 9 scale variations & 103 PDF variations, which should hold for most of the cases.
Demo of Relval plots with workflow 556 (TTbar Powheg+Pythia8) would look like this.
PR validation:
Tested with following GEN workflows:
as the routine will run for all GEN workflows, there should be no exception regardless of having LHE products or not.