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

Update hcal strip 80 x #13117

Merged
merged 4 commits into from
Feb 1, 2016
Merged

Conversation

aminnj
Copy link
Contributor

@aminnj aminnj commented Jan 29, 2016

The Hcal Strip beam halo filter from #10934 has been supplemented with two extra cuts to slightly increase halo rejection efficiency while dramatically lowering fake rate (O(10^-4)) in MC signal samples with large amounts of hadronic activity.

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @aminnj (Nick Amin) for CMSSW_8_0_X.

It involves the following packages:

DataFormats/METReco
RecoMET/METAlgorithms
RecoMET/METFilters

@cvuosalo, @monttj, @cmsbuild, @slava77, @vadler, @davidlange6 can you please review it and eventually sign? Thanks.
@rappoccio, @ahinzmann, @mmarionncern, @jdolen, @nhanvtran, @schoef, @mariadalfonso, @TaiSakuma 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

@slava77
Copy link
Contributor

slava77 commented Jan 29, 2016

@aminnj
Please provide a link to indico if there was a presentation in a POG meeting on this topic.

std::vector<const CaloTower*> sortedCaloTowers;
for(CaloTowerCollection::const_iterator tower = TheCaloTowers->begin(); tower != TheCaloTowers->end(); tower++) {
if(abs(tower->ieta()) <= maxAbsIEta && tower->numProblematicHcalCells() > 0)
sortedCaloTowers.push_back(&(*tower));
if(abs(tower->ieta()) > maxAbsIEta) continue;
Copy link
Contributor

Choose a reason for hiding this comment

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

std::abs (to more convincingly see it's the C++ STL abs, not the C abs)

Copy link
Contributor

Choose a reason for hiding this comment

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

oops, it's ieta
nevermind

@cmsbuild
Copy link
Contributor

Pull request #13117 was updated. @cvuosalo, @monttj, @cmsbuild, @slava77, @vadler, @davidlange6 can you please check and sign again.

@slava77
Copy link
Contributor

slava77 commented Jan 29, 2016

@cmsbuild please test

@cmsbuild
Copy link
Contributor

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

@cmsbuild
Copy link
Contributor

@slava77
Copy link
Contributor

slava77 commented Jan 29, 2016

@cmsbuild please test
tests and comparisons were run with an IB older than the tested code baseline.
Better get a clean set of comparisons to check

@slava77
Copy link
Contributor

slava77 commented Jan 30, 2016

+1

for #13117 69d3376

  • code changes are in line with the description
  • jenkins tests pass and comparisons with baseline show no difference ; local copy of jenkins outputs shows differences only in BeamHaloData and in HcalHaloData
  • local comparisons with halo plots added show changes in halo (taking 134.911): shorter problematic cells are removed and there less energetic ones are gone
    all_13117-10856vsbaseline_runsingleph2015dwf134p911c_recohcalhalodata_hcalhalodata__rereco_obj_getproblematicstrips_celltoweridsat_size

all_13117-10856vsbaseline_runsingleph2015dwf134p911c_recohcalhalodata_hcalhalodata__rereco_obj_getproblematicstrips_hadet

@slava77
Copy link
Contributor

slava77 commented Jan 30, 2016

(plots were updated in #13117 (comment))

@@ -7,6 +7,7 @@
[date]: October 15, 2009
*/
#include <vector>
#include <map>
Copy link
Contributor

Choose a reason for hiding this comment

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

this include should be in the cc file (though see next comment)
@aminnj

Copy link
Contributor

Choose a reason for hiding this comment

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

good catch

@cmsbuild
Copy link
Contributor

Pull request #13117 was updated. @cvuosalo, @monttj, @cmsbuild, @slava77, @vadler, @davidlange6 can you please check and sign again.

@slava77
Copy link
Contributor

slava77 commented Jan 31, 2016

@cmsbuild please test

@cmsbuild
Copy link
Contributor

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

@cmsbuild
Copy link
Contributor

-1

Tested at: a468287
I found errors in the following unit tests:

---> test testRecoMETMETProducers had ERRORS

you can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-13117/10867/summary.html

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 1, 2016

@slava77
Copy link
Contributor

slava77 commented Feb 1, 2016

+1

for #13117 a468287

davidlange6 added a commit that referenced this pull request Feb 1, 2016
@davidlange6 davidlange6 merged commit cecda2a into cms-sw:CMSSW_8_0_X Feb 1, 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.

4 participants