Make Cosmic superpointer module able to select muons also based on number of interactions with tracker or muon chambers #26511

NTrevisani · 2019-04-23T16:25:33Z

PR description:

Introducing the possibility of selecting based on number of valid pixel hits, number of tracker layers with interactions, and number of valid muon hits. The old version only required the muon to pass through a cylinder of variable size centered at the center of CMS.

PR validation:

Comparing the output of the skim with the old one and checking that new selections work.

if this PR is a backport please specify the original PR:

Before submitting your pull requests, make sure you followed this checklist:

verify that the PR is really intended for the chosen branch
verify that changes follow CMS Naming, Coding, And Style Rules
verify that the PR passes the basic test procedure suggested in the CMSSW PR instructions

cmsbuild · 2019-04-23T16:26:06Z

The code-checks are being triggered in jenkins.

NTrevisani · 2019-04-23T16:26:23Z

Introducing new features in cosmicSP for cosmic muons certification.

cmsbuild · 2019-04-23T16:33:08Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-26511/9374

This PR adds an extra 20KB to repository

cmsbuild · 2019-04-23T16:33:41Z

A new Pull Request was created by @NTrevisani for master.

It involves the following packages:

DPGAnalysis/Skims
HLTrigger/special

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

cms-bot commands are listed here

fwyzard · 2019-04-23T16:44:51Z

HLTrigger/special/plugins/HLTMuonPointingFilter.cc

@@ -39,6 +39,9 @@ HLTMuonPointingFilter::HLTMuonPointingFilter(const edm::ParameterSet& pset) :
  thePropagatorName(pset.getParameter<std::string>("PropagatorName") ),
  theRadius(        pset.getParameter<double>("radius") ),              // cyl's radius (cm)
  theMaxZ(          pset.getParameter<double>("maxZ") ),                // cyl's half lenght (cm)
+  thenPixHits(      pset.getParameter<int>("nPixHits") ),               // pixel hits
+  theTkLayers(      pset.getParameter<int>("TkLayers") ),               // tracker layers with measurements
+  thenMuonHits(     pset.getParameter<int>("nMuonHits") ),              // tracker layers with measurements


the comment looks wrong ?

fwyzard · 2019-04-23T16:47:07Z

HLTrigger/special/plugins/HLTMuonPointingFilter.cc

@@ -104,8 +116,17 @@ bool HLTMuonPointingFilter::filter(edm::Event& event, const edm::EventSetup& eve
    if ( tsosAtCyl.isValid() ) {
      LogDebug("HLTMuonPointing") << " extrap TSOS " << tsosAtCyl;


I think it would make more sense to move all LogDebugs here, rather than inside the ifs

fwyzard · 2019-04-23T16:48:23Z

HLTrigger/special/plugins/HLTMuonPointingFilter.cc

@@ -39,6 +39,9 @@ HLTMuonPointingFilter::HLTMuonPointingFilter(const edm::ParameterSet& pset) :
  thePropagatorName(pset.getParameter<std::string>("PropagatorName") ),
  theRadius(        pset.getParameter<double>("radius") ),              // cyl's radius (cm)
  theMaxZ(          pset.getParameter<double>("maxZ") ),                // cyl's half lenght (cm)
+  thenPixHits(      pset.getParameter<int>("nPixHits") ),               // pixel hits


Can you make the new parameters unsigned ?
I assume it makes no sense to ask for > -1 hits, since 0 means no requirements, right ?

fwyzard · 2019-04-23T16:49:48Z

HLTrigger/special/plugins/HLTMuonPointingFilter.cc

@@ -117,8 +138,17 @@ bool HLTMuonPointingFilter::filter(edm::Event& event, const edm::EventSetup& eve

 	if (tsosAtPlane.isValid()){
 	  if (tsosAtPlane.globalPosition().perp()< theRadius){


as above, can you move here all LogDebugs ?

fwyzard · 2019-04-23T16:50:01Z

HLTrigger/special/plugins/HLTMuonPointingFilter.cc

@@ -140,6 +170,9 @@ void HLTMuonPointingFilter::fillDescriptions(edm::ConfigurationDescriptions & de
  desc.add<std::string>("PropagatorName", "SteppingHelixPropagatorAny");
  desc.add<double>("radius", 90.0);
  desc.add<double>("maxZ", 280.0);
+  desc.add<int>("nPixHits",  0);


as above, could you make these unsigned ?

fwyzard · 2019-04-23T16:50:22Z

HLTrigger/special/plugins/HLTMuonPointingFilter.h

@@ -49,6 +49,9 @@ class HLTMuonPointingFilter : public edm::EDFilter {
  const std::string thePropagatorName;      // name of propagator to be used
  const double theRadius;                   // radius of cylinder
  const double theMaxZ;                     // half length of cylinder
+  const int    thenPixHits;                 // number of pixel hits


as in the .cc file, could you make these unsigned ?

fwyzard · 2019-04-23T16:51:07Z

Can you give a more descriptive name to the pull request ?

cmsbuild · 2019-04-23T21:41:56Z

The code-checks are being triggered in jenkins.

cmsbuild · 2019-04-23T21:49:30Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-26511/9383

This PR adds an extra 24KB to repository

cmsbuild · 2019-04-23T21:50:01Z

Pull request #26511 was updated. @Martin-Grunewald, @prebello, @cmsbuild, @zhenhu, @pgunnell, @fwyzard can you please check and sign again.

fwyzard · 2019-04-23T23:37:56Z

OK, but still... what is a "Cosmic SP" ?

slava77 · 2019-04-24T02:19:33Z

I guess it's a super-pointing (SP) skim/selection for cosmics (a muon crosses pixels; although historically exact defs varied)

Martin-Grunewald · 2019-04-24T06:29:06Z

HLTrigger/special/plugins/HLTMuonPointingFilter.cc

@@ -39,6 +39,9 @@ HLTMuonPointingFilter::HLTMuonPointingFilter(const edm::ParameterSet& pset) :
  thePropagatorName(pset.getParameter<std::string>("PropagatorName") ),
  theRadius(        pset.getParameter<double>("radius") ),              // cyl's radius (cm)
  theMaxZ(          pset.getParameter<double>("maxZ") ),                // cyl's half lenght (cm)
+  thenPixHits(      pset.getParameter<unsigned int>("nPixHits") ),      // pixel hits
+  theTkLayers(      pset.getParameter<unsigned int>("TkLayers") ),      // tracker layers with measurements
+  thenMuonHits(     pset.getParameter<unsigned int>("nMuonHits") ),     // muon hits
  thePropagator(nullptr),


The new variables are all integer numbers, counting something.
Why are two of them called nSomething, while the third is no 'n'?
I also find name thenMuonHits confusing, why 'then' :)?
I suggest theNmuonHits, or theMuonHits, but at least make it consistent between the variables.

cmsbuild · 2019-04-24T14:21:46Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-26511/9390

This PR adds an extra 24KB to repository

cmsbuild · 2019-04-24T14:22:26Z

Pull request #26511 was updated. @Martin-Grunewald, @prebello, @cmsbuild, @zhenhu, @pgunnell, @fwyzard can you please check and sign again.

NTrevisani · 2019-04-25T12:22:52Z

Hi,

it looks like the last automatic tests were not successful due to an eos issue, which made the rootfiles impossible to be read.
May I ask you to trigger the tests again, please?

Cheers,
Nicolò

Martin-Grunewald · 2019-04-26T08:16:35Z

please test

cmsbuild · 2019-04-26T08:17:11Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/34354/console Started: 2019/04/26 10:17

cmsbuild · 2019-04-26T09:44:20Z

+1
Tested at: 9e3e968
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-26511/34354/summary.html

cmsbuild · 2019-04-26T09:44:26Z

Comparison job queued.

cmsbuild · 2019-04-26T11:38:45Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-26511/34354/summary.html

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 33
DQMHistoTests: Total histograms compared: 3211964
DQMHistoTests: Total failures: 1
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3211759
DQMHistoTests: Total skipped: 204
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 32 files compared)
Checked 137 log files, 14 edm output root files, 33 DQM output files

Martin-Grunewald · 2019-04-27T10:47:44Z

+1

fabiocos · 2019-04-29T12:19:52Z

@prebello @zhenhu @kfjack could you please have a look? The PdmV part of this PR seems an obvious adjustment for the module parameters update

prebello · 2019-04-30T18:56:23Z

+1

cmsbuild · 2019-04-30T18:56:52Z

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. @davidlange6, @slava77, @smuzaffar, @fabiocos (and backports should be raised in the release meeting by the corresponding L2)

fabiocos · 2019-04-30T20:32:04Z

+1

NTrevisani added 2 commits April 23, 2019 18:15

new features in cosmicSP

574c4f6

new features in cosmicSP

263b9a2

cmsbuild added this to the CMSSW_10_6_X milestone Apr 23, 2019

cmsbuild added code-checks-pending comparison-pending hlt-pending orp-pending pdmv-pending pending-signatures tests-pending labels Apr 23, 2019

cmsbuild added code-checks-approved and removed code-checks-pending labels Apr 23, 2019

fwyzard reviewed Apr 23, 2019

View reviewed changes

fixing LogDebugs and uint

6cc67a6

cmsbuild added code-checks-pending and removed code-checks-approved labels Apr 23, 2019

NTrevisani changed the title ~~My cosmic sp~~ Make Cosmic SP able to select muons also based on number of interactions with tracker or muon chambers Apr 23, 2019

cmsbuild added code-checks-approved and removed code-checks-pending labels Apr 23, 2019

Martin-Grunewald reviewed Apr 24, 2019

View reviewed changes

cmsbuild added code-checks-approved and removed code-checks-pending labels Apr 24, 2019

cmsbuild added tests-started and removed tests-pending labels Apr 26, 2019

cmsbuild added tests-approved and removed tests-started labels Apr 26, 2019

cmsbuild added comparison-available and removed comparison-pending labels Apr 26, 2019

cmsbuild added hlt-approved and removed hlt-pending labels Apr 27, 2019

cmsbuild added fully-signed pdmv-approved and removed pdmv-pending pending-signatures labels Apr 30, 2019

cmsbuild added orp-approved and removed orp-pending labels Apr 30, 2019

cmsbuild merged commit 8e9df5e into cms-sw:master Apr 30, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make Cosmic superpointer module able to select muons also based on number of interactions with tracker or muon chambers #26511

Make Cosmic superpointer module able to select muons also based on number of interactions with tracker or muon chambers #26511

NTrevisani commented Apr 23, 2019 •

edited

cmsbuild commented Apr 23, 2019

NTrevisani commented Apr 23, 2019

cmsbuild commented Apr 23, 2019

cmsbuild commented Apr 23, 2019

fwyzard Apr 23, 2019

fwyzard Apr 23, 2019

fwyzard Apr 23, 2019

fwyzard Apr 23, 2019

fwyzard Apr 23, 2019

fwyzard Apr 23, 2019

fwyzard commented Apr 23, 2019

cmsbuild commented Apr 23, 2019

cmsbuild commented Apr 23, 2019

cmsbuild commented Apr 23, 2019

fwyzard commented Apr 23, 2019

slava77 commented Apr 24, 2019

Martin-Grunewald Apr 24, 2019

cmsbuild commented Apr 24, 2019

cmsbuild commented Apr 24, 2019

NTrevisani commented Apr 25, 2019

Martin-Grunewald commented Apr 26, 2019

cmsbuild commented Apr 26, 2019 •

edited

cmsbuild commented Apr 26, 2019

cmsbuild commented Apr 26, 2019

cmsbuild commented Apr 26, 2019

Martin-Grunewald commented Apr 27, 2019

fabiocos commented Apr 29, 2019

prebello commented Apr 30, 2019

cmsbuild commented Apr 30, 2019

fabiocos commented Apr 30, 2019

		@@ -104,8 +116,17 @@ bool HLTMuonPointingFilter::filter(edm::Event& event, const edm::EventSetup& eve
		if ( tsosAtCyl.isValid() ) {
		LogDebug("HLTMuonPointing") << " extrap TSOS " << tsosAtCyl;

		@@ -117,8 +138,17 @@ bool HLTMuonPointingFilter::filter(edm::Event& event, const edm::EventSetup& eve

		if (tsosAtPlane.isValid()){
		if (tsosAtPlane.globalPosition().perp()< theRadius){

Make Cosmic superpointer module able to select muons also based on number of interactions with tracker or muon chambers #26511

Make Cosmic superpointer module able to select muons also based on number of interactions with tracker or muon chambers #26511

Conversation

NTrevisani commented Apr 23, 2019 • edited

PR description:

PR validation:

if this PR is a backport please specify the original PR:

cmsbuild commented Apr 23, 2019

NTrevisani commented Apr 23, 2019

cmsbuild commented Apr 23, 2019

cmsbuild commented Apr 23, 2019

fwyzard Apr 23, 2019

Choose a reason for hiding this comment

fwyzard Apr 23, 2019

Choose a reason for hiding this comment

fwyzard Apr 23, 2019

Choose a reason for hiding this comment

fwyzard Apr 23, 2019

Choose a reason for hiding this comment

fwyzard Apr 23, 2019

Choose a reason for hiding this comment

fwyzard Apr 23, 2019

Choose a reason for hiding this comment

fwyzard commented Apr 23, 2019

cmsbuild commented Apr 23, 2019

cmsbuild commented Apr 23, 2019

cmsbuild commented Apr 23, 2019

fwyzard commented Apr 23, 2019

slava77 commented Apr 24, 2019

Martin-Grunewald Apr 24, 2019

Choose a reason for hiding this comment

cmsbuild commented Apr 24, 2019

cmsbuild commented Apr 24, 2019

NTrevisani commented Apr 25, 2019

Martin-Grunewald commented Apr 26, 2019

cmsbuild commented Apr 26, 2019 • edited

cmsbuild commented Apr 26, 2019

cmsbuild commented Apr 26, 2019

cmsbuild commented Apr 26, 2019

Martin-Grunewald commented Apr 27, 2019

fabiocos commented Apr 29, 2019

prebello commented Apr 30, 2019

cmsbuild commented Apr 30, 2019

fabiocos commented Apr 30, 2019

NTrevisani commented Apr 23, 2019 •

edited

cmsbuild commented Apr 26, 2019 •

edited