make track quality requirement in GeneralTracksImporter and PFTrackAl… #28200

jsalfeld · 2019-10-17T13:04:43Z

…goTools configurable

PR description:

Track Quality requirements in the PFBlockProducer are hardcoded. This PR is intended add configurability.

PR validation:

I ran OnLine_HLT_GRun.py with the new input for different track quality requirements in "process.hltParticleFlowBlock" PFBlockProducer. Locally I did a print out to check if bools are set correctly and the string correctly passed.

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

…goTools configurable

cmsbuild · 2019-10-17T13:05:14Z

The code-checks are being triggered in jenkins.

cmsbuild · 2019-10-17T13:11:22Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-28200/12305

This PR adds an extra 20KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File RecoParticleFlow/PFProducer/plugins/importers/GeneralTracksImporter.cc modified in PR(s): Reconfigure particle flow config files #28110

Code check has found code style and quality issues which could be resolved by applying following patch(s)

code-format:
https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-28200/12305/code-format.patch
e.g. curl https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-28200/12305/code-format.patch | patch -p1
You can also run scram build code-format to apply code format directly

fwyzard · 2019-10-17T13:11:32Z

RecoParticleFlow/PFProducer/plugins/importers/GeneralTracksImporter.cc

@@ -31,10 +33,10 @@ class GeneralTracksImporter : public BlockElementImporterBase {

  edm::EDGetTokenT<reco::PFRecTrackCollection> src_;
  edm::EDGetTokenT<reco::MuonCollection> muons_;
+  std::string trackQuality_;


could you make this const ?

fwyzard · 2019-10-17T13:12:57Z

RecoParticleFlow/PFTracking/interface/PFTrackAlgoTools.h

@@ -18,5 +18,6 @@ namespace PFTrackAlgoTools {
                        const std::vector<double>& DPtovPtCut,
                        const std::vector<unsigned>& NHitCut,
                        bool useIterTracking,
-                        bool debug = false);
+                        bool debug = false,
+			std::string trackQuality = "highPurity");


I would suggest to swap the order, and have trackQuality before debug.

Also

Suggested change

std::string trackQuality = "highPurity");

const std::string& trackQuality = "highPurity");

The change of order would complicate backward compatibility and introduce changes to where this function ("goodPtResolution") is used without trackQuality variable as input explicitly.

Let me know if this is ok (in principle I agree that having "debug" as last variable).

As far as I can tell, the affected classes are these ones:
RecoParticleFlow/PFTracking/plugins/HGCalTrackCollectionProducer.cc
RecoParticleFlow/PFProducer/plugins/importers/GeneralTracksImporterWithVeto.cc

@makortel is your suggested change ready to go into the commit or needs testing?
@fwyzard let me know if the other classes should be changed to accommodate the suggested change of order

I think you can find out by running git cms-checkdeps -a, rebuilding with scram b, and seeing what breaks...

makortel · 2019-10-17T13:46:40Z

RecoParticleFlow/PFProducer/plugins/importers/GeneralTracksImporter.cc

@@ -15,6 +15,8 @@ class GeneralTracksImporter : public BlockElementImporterBase {
      : BlockElementImporterBase(conf, sumes),
        src_(sumes.consumes<reco::PFRecTrackCollection>(conf.getParameter<edm::InputTag>("source"))),
        muons_(sumes.consumes<reco::MuonCollection>(conf.getParameter<edm::InputTag>("muonSrc"))),
+	trackQuality_(
+	  (conf.existsAs<std::string>("trackQuality")) ? conf.getParameter<std::string>("trackQuality") : trackQuality_ = "highPurity"),


Suggested change

(conf.existsAs<std::string>("trackQuality")) ? conf.getParameter<std::string>("trackQuality") : trackQuality_ = "highPurity"),

(conf.existsAs<std::string>("trackQuality")) ? conf.getParameter<std::string>("trackQuality") : "highPurity"),

@makortel is your suggested change ready to go into the commit or needs testing?

makortel · 2019-10-17T13:46:44Z

RecoParticleFlow/PFTracking/interface/PFTrackAlgoTools.h

@@ -18,5 +18,6 @@ namespace PFTrackAlgoTools {
                        const std::vector<double>& DPtovPtCut,
                        const std::vector<unsigned>& NHitCut,
                        bool useIterTracking,
-                        bool debug = false);
+                        bool debug = false,
+			std::string trackQuality = "highPurity");


Also

Suggested change

std::string trackQuality = "highPurity");

const std::string& trackQuality = "highPurity");

makortel · 2019-10-17T13:49:18Z

RecoParticleFlow/PFTracking/src/PFTrackAlgoTools.cc

+			std::string trackQuality) {
+
+    //check quality of tracks
+    if(!trackref->quality(reco::TrackBase::qualityByName(trackQuality)))


Actually, it would be better to do the string-to-enum conversion (reco::TrackBase::qualityByName()) once in the constructor (and possibly throw an exception if the returned value is undefQuality), and pass the enum to this function.

@makortel apart from the last comment, is your suggested change ready to go into the commit or needs testing?

Changing the trackQuality parameter here and GeneralTracksImporter::trackQuality_ to TrackBase::TrackQuality enum? Yes, please do (and of course it needs to be eventually tested).

hatakeyamak · 2019-10-17T14:14:56Z

Thanks Jakob!

Perhaps this is beyond the scope of this PR, but it might be nice if we can move these config parameter setting for PFBlockProducer to fillDescription-based, as I am doing for PFProducer on #28110
(https://twiki.cern.ch/twiki/bin/view/CMSPublic/SWGuideConfigurationValidationAndHelp)
which allow us to validate input parameters. Just wanted to mention this to see if this is something RECO conveners would prefer and Jakob would be willing to work on.

…rder of debug and trackquality, no exception thrown though

cmsbuild · 2019-10-18T22:06:12Z

The code-checks are being triggered in jenkins.

cmsbuild · 2019-10-18T22:13:59Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-28200/12339

This PR adds an extra 24KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File RecoParticleFlow/PFProducer/plugins/importers/GeneralTracksImporter.cc modified in PR(s): Reconfigure particle flow config files #28110
- File RecoParticleFlow/PFProducer/plugins/importers/GeneralTracksImporterWithVeto.cc modified in PR(s): Reconfigure particle flow config files #28110

cmsbuild · 2019-10-18T22:14:30Z

A new Pull Request was created by @jsalfeld (Jakob Salfeld-Nebgen) for master.

It involves the following packages:

RecoParticleFlow/PFProducer
RecoParticleFlow/PFTracking

@perrotta, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@mmarionncern, @makortel, @cbernet, @rovere, @lgray, @bachtis, @hatakeyamak, @seemasharmafnal 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

slava77 · 2019-10-18T22:19:33Z

@cmsbuild please test

cmsbuild · 2019-10-18T22:19:55Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/3034/console Started: 2019/10/19 00:20

cmsbuild · 2019-10-19T00:08:05Z

+1
Tested at: 7f08dd6
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-2ee0e7/3034/summary.html

cmsbuild · 2019-10-19T00:08:08Z

Comparison job queued.

cmsbuild · 2019-10-28T15:47:13Z

The code-checks are being triggered in jenkins.

cmsbuild · 2019-10-28T15:52:55Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-28200/12478

This PR adds an extra 32KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File RecoParticleFlow/PFProducer/plugins/importers/GeneralTracksImporter.cc modified in PR(s): Reconfigure particle flow config files #28110
- File RecoParticleFlow/PFProducer/plugins/importers/GeneralTracksImporterWithVeto.cc modified in PR(s): Reconfigure particle flow config files #28110

cmsbuild · 2019-10-28T15:53:24Z

Pull request #28200 was updated. @perrotta, @cmsbuild, @slava77 can you please check and sign again.

perrotta · 2019-10-28T15:57:38Z

please test

cmsbuild · 2019-10-28T15:58:09Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/3202/console Started: 2019/10/28 16:58

hatakeyamak · 2019-10-28T16:39:09Z

yes indeed, will do asap. I assume these things will be fixed once the fillDescription is used and the cfi`s are produced automatically.

Thanks for sorting this out.

cmsbuild · 2019-10-28T17:22:23Z

+1
Tested at: 9173120
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-2ee0e7/3202/summary.html

cmsbuild · 2019-10-28T17:22:27Z

Comparison job queued.

cmsbuild · 2019-10-28T19:02:23Z

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

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 4 differences found in the comparisons
DQMHistoTests: Total files compared: 34
DQMHistoTests: Total histograms compared: 2961036
DQMHistoTests: Total failures: 2
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2960693
DQMHistoTests: Total skipped: 341
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 33 files compared)
Checked 147 log files, 16 edm output root files, 34 DQM output files

perrotta · 2019-10-28T20:26:33Z

+1

The trackQuality to compare with is now made configurable in the mentioned classes
Some cleaning of the code and python configurations is also implemented
Jenkins tests pass and show no difference

cmsbuild · 2019-10-28T20:27:06Z

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-10-29T21:52:12Z

+1

Martin-Grunewald · 2019-10-30T05:33:56Z

@perrotta
Just to confirm: PFBlockProducer is currently parsed from RecoParticleFlow/PFProducer/python/particleFlowBlock_cfi.py
But the two parameters you mention are NOT top-level parameters but inside nested PSets, and so they would need to be added by hand in existing ConfDB instances of the module.

make track quality requirement in GeneralTracksImporter and PFTrackAl…

f40b395

…goTools configurable

cmsbuild added this to the CMSSW_11_0_X milestone Oct 17, 2019

cmsbuild added code-checks-pending comparison-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Oct 17, 2019

cmsbuild added code-checks-rejected and removed code-checks-pending labels Oct 17, 2019

fwyzard reviewed Oct 17, 2019

View reviewed changes

makortel reviewed Oct 17, 2019

View reviewed changes

adjust according to suggestions, string enum in constructer, change o…

7f08dd6

…rder of debug and trackquality, no exception thrown though

cmsbuild added code-checks-pending and removed code-checks-rejected labels Oct 18, 2019

cmsbuild added code-checks-approved and removed code-checks-pending labels Oct 18, 2019

cmsbuild added tests-started and removed tests-pending labels Oct 18, 2019

cmsbuild removed the tests-started label Oct 19, 2019

cmsbuild added code-checks-pending and removed code-checks-approved labels Oct 28, 2019

cmsbuild added code-checks-approved and removed code-checks-pending labels Oct 28, 2019

cmsbuild added tests-started and removed tests-pending labels Oct 28, 2019

cmsbuild added tests-approved and removed tests-started labels Oct 28, 2019

cmsbuild added comparison-available and removed comparison-pending labels Oct 28, 2019

cmsbuild added fully-signed reconstruction-approved and removed pending-signatures reconstruction-pending labels Oct 28, 2019

cmsbuild added orp-approved and removed orp-pending labels Oct 29, 2019

cmsbuild merged commit 4e01538 into cms-sw:master Oct 29, 2019

	std::string trackQuality = "highPurity");
	const std::string& trackQuality = "highPurity");

	(conf.existsAs<std::string>("trackQuality")) ? conf.getParameter<std::string>("trackQuality") : trackQuality_ = "highPurity"),
	(conf.existsAs<std::string>("trackQuality")) ? conf.getParameter<std::string>("trackQuality") : "highPurity"),

make track quality requirement in GeneralTracksImporter and PFTrackAl… #28200

make track quality requirement in GeneralTracksImporter and PFTrackAl… #28200

Conversation

jsalfeld commented Oct 17, 2019

PR description:

PR validation:

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

cmsbuild commented Oct 17, 2019

cmsbuild commented Oct 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fwyzard Oct 17, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hatakeyamak commented Oct 17, 2019 • edited

cmsbuild commented Oct 18, 2019

cmsbuild commented Oct 18, 2019

cmsbuild commented Oct 18, 2019

slava77 commented Oct 18, 2019

cmsbuild commented Oct 18, 2019 • edited

cmsbuild commented Oct 19, 2019

cmsbuild commented Oct 19, 2019

cmsbuild commented Oct 28, 2019

cmsbuild commented Oct 28, 2019

cmsbuild commented Oct 28, 2019

perrotta commented Oct 28, 2019

cmsbuild commented Oct 28, 2019 • edited

hatakeyamak commented Oct 28, 2019

cmsbuild commented Oct 28, 2019

cmsbuild commented Oct 28, 2019

cmsbuild commented Oct 28, 2019

perrotta commented Oct 28, 2019

cmsbuild commented Oct 28, 2019

fabiocos commented Oct 29, 2019

Martin-Grunewald commented Oct 30, 2019 • edited

fwyzard Oct 17, 2019 •

edited

hatakeyamak commented Oct 17, 2019 •

edited

cmsbuild commented Oct 18, 2019 •

edited

cmsbuild commented Oct 28, 2019 •

edited

Martin-Grunewald commented Oct 30, 2019 •

edited