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

Fix deltaR calculation in HLT widejet module #14587

Merged
merged 1 commit into from May 22, 2016
Merged

Fix deltaR calculation in HLT widejet module #14587

merged 1 commit into from May 22, 2016

Conversation

duanders
Copy link
Contributor

Modifies HLTFatJetMassFilter so that it computes DeltaR correctly. (The current behavior does not treat DeltaPhi modulo 2 pi.)

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @duanders (Dustin Anderson) for CMSSW_8_0_X.

It involves the following packages:

HLTrigger/JetMET

@Martin-Grunewald, @perrotta, @cmsbuild, @fwyzard, @davidlange6 can you please review it and eventually sign? Thanks.
@Martin-Grunewald this is something you requested to watch as well.
@slava77, @Degano, @smuzaffar you are the release manager for this.

cms-bot commands are list here #13028

@fwyzard
Copy link
Contributor

fwyzard commented May 20, 2016

please test

@Martin-Grunewald
Copy link
Contributor

need a 81X PR as well!

@cmsbuild
Copy link
Contributor

cmsbuild commented May 20, 2016

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/13102/console

@@ -128,11 +129,11 @@ HLTFatJetMassFilter<jetType>::hltFilter(edm::Event& iEvent, const edm::EventSetu

// apply radiation recovery
for ( recojet = recojets.begin() ; recojet != recojets.end() ; recojet++) {
double DeltaR1 = sqrt(pow(recojet->phi()-j1.phi(), 2.)+pow(recojet->eta()-j1.eta(),2.));
double DeltaR2 = sqrt(pow(recojet->phi()-j2.phi(), 2.)+pow(recojet->eta()-j2.eta(),2.));
if(DeltaR1 < DeltaR2 && DeltaR1 < fatJetDeltaR_) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@duanders from the code, I think the bug you are trying to fix is that |phi2 - phi1| does not properly wrap around π , right ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fwyzard yes that's right.

@fwyzard
Copy link
Contributor

fwyzard commented May 20, 2016

+1

@fwyzard
Copy link
Contributor

fwyzard commented May 20, 2016

I think this should be simple and safe enough to enter directly as bugfix in 8.0.x - @davidlange6 what do you say ?

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_8_0_X IBs after it passes the integration tests. This pull request requires discussion in the ORP meeting before it's merged. @slava77, @davidlange6, @Degano, @smuzaffar

@davidlange6
Copy link
Contributor

it can go into 80x, but would like an 81x request too. (also for the second request from @duanders )

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

@davidlange6 davidlange6 merged commit 88ae137 into cms-sw:CMSSW_8_0_X May 22, 2016
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

5 participants