-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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 to PUPPI;speedup, low PU corr #11671
Conversation
A new Pull Request was created by @nhanvtran for CMSSW_7_6_X. update to PUPPI;speedup, low PU corr It involves the following packages: CommonTools/PileupAlgos @cmsbuild, @cvuosalo, @vadler, @monttj, @slava77 can you please review it and eventually sign? Thanks. |
@@ -47,11 +47,7 @@ void PuppiContainer::initialize(const std::vector<RecoObj> &iRecoObjects) { | |||
// float nom = sqrt((fRecoParticle.m)*(fRecoParticle.m) + (fRecoParticle.pt)*(fRecoParticle.pt)*(cosh(fRecoParticle.eta))*(cosh(fRecoParticle.eta))) + (fRecoParticle.pt)*sinh(fRecoParticle.eta);//hacked | |||
// float denom = sqrt((fRecoParticle.m)*(fRecoParticle.m) + (fRecoParticle.pt)*(fRecoParticle.pt));//hacked | |||
// float rapidity = log(nom/denom);//hacked | |||
if (edm::isFinite(fRecoParticle.rapidity)){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please recover this isFinite check.
I'm OK if some other "random" is picked instead of reset_PtYPhiM(0, 99., 0, 0)
or if it can be skipped,
but we should not let the pseudojet to crash on nans again
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@slava77 sorry it got dropped -- I put it back in
@nhanvtran ping |
@slava77 |
|
||
} | ||
//////////////////////////////////////////////////////////////////////////////// | ||
|
||
//This code is probably a bit confusing | ||
double PuppiAlgo::compute(std::vector<double> const &iVals,double iChi2) const { | ||
// if(fAlgoId[0] == -1) return 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remind me, please why this commented out code is needed here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@slava77 deleted
On 10/19/15 2:48 PM, nhanvtran wrote:
Yes, you need to put the if with isFinite back in.
|
@slava77 |
@nhanvtran Thanks. |
@slava77 |
@cmsbuild please test |
The tests are being triggered in jenkins. |
double lAdjust = double(lNPV)/double(lNPV+0.5*fNCount[iAlgo]); | ||
if(lAdjust > 0) { | ||
fMedian[iAlgo] -= sqrt(ROOT::Math::chisquared_quantile(lAdjust,1.)*fRMS[iAlgo]); | ||
fRMS[iAlgo] -= sqrt(ROOT::Math::chisquared_quantile(lAdjust,1.)*fRMS[iAlgo]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it looks a bit odd that both the median and RMS are shifted by the same value.
Is this done correctly?
Also, copy-paste of the same multi-operand code is not good (use a temporary).
+1
|
update to PUPPI;speedup, low PU corr
This update contains the code for PUPPI v9
Biggest updates are a speedup (and more speedup possibility in the future) and fixes for the low PU behavior.
Final v9 version will be ready after tuning is performed with the new HCAL/ECAL reconstruction fixes and new constants are committed.