Modification of cone based tau algorithm to furbish it as fallback solution for the HLT for upcoming data taking #18429

roger-wolf · 2017-04-20T22:49:30Z

Dear colleagues,

this is a pull request by @mbluj that contains modifications of the cone based tau reconstruction algorithm as used for the HLT to make it fit for upcoming data taking periods, as fallback in case that the planned HPS-port to the HLT won't be ready in time. I'm citing Michal's comments (*) and refer to him for further questions.

Cheers,
Roger

(*)
This PR is to modify the Cone-based tau builder (used at HLT) to fill some additional data members of produced PFTau not filled so far:

Size of signal cone (cone for PiZeros used);
Pt carried by signal PiZeros outside of the signal cone; it is important shape variable supporting isolation in rejecting jet->tau fakes;
Charge, pdgId, tau decay mode hypothesis.

The changes do not modify current HLT work-flow (the builder is used only at HLT), but provides new features (cf. 2) allowing better jet->tau fake rejection during 2017 data-taking.

This is PR to 91X on top of changes proposed for tau vertex association (cf. #18243), a separate PR for 90X will follow shortly.

Modified Cone-based Tau Builder

…pog_technical-developments_coneBasedTaus

cmsbuild · 2017-04-20T22:49:48Z

A new Pull Request was created by @roger-wolf (Roger Wolf) for master.

It involves the following packages:

RecoTauTag/RecoTau

@perrotta, @cmsbuild, @slava77, @davidlange6 can you please review it and eventually sign? Thanks.
@Muzaffar, @davidlange6, @smuzaffar you are the release manager for this.

cms-bot commands are listed here

slava77 · 2017-04-20T22:56:34Z

@cmsbuild please test

cmsbuild · 2017-04-20T22:56:50Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/19309/console Started: 2017/04/21 01:11

cmsbuild · 2017-04-21T00:24:28Z

+1
Tested at: 7c6e96e
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-18429/19309/summary.html

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:
7c6e96e
199f427
You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-18429/19309/git-log-recent-commits
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-18429/19309/git-merge-result

cmsbuild · 2017-04-21T00:24:31Z

Comparison job queued.

cmsbuild · 2017-04-21T01:52:30Z

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

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 1648 differences found in the comparisons
DQMHistoTests: Total files compared: 23
DQMHistoTests: Total histograms compared: 1826273
DQMHistoTests: Total failures: 6234
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 1819866
DQMHistoTests: Total skipped: 173
DQMHistoTests: Total Missing objects: 0
Checked 94 log files, 14 edm output root files, 23 DQM output files

perrotta · 2017-04-24T11:34:06Z

RecoTauTag/RecoTau/plugins/RecoTauBuilderConePlugin.cc

+  int charge = 0;
+  int leadCharge = aTau.leadPFChargedHadrCand().isNonnull() ? aTau.leadPFChargedHadrCand()->charge() : 0;
+  const std::vector<reco::PFCandidatePtr>& pfChs = aTau.signalPFChargedHadrCands();
+  for(std::vector<reco::PFCandidatePtr>::const_iterator pfCh = pfChs.begin();


Range based for loops must be preferred

perrotta · 2017-04-24T11:34:46Z

RecoTauTag/RecoTau/plugins/RecoTauBuilderConePlugin.cc

+      piZero != piZeros.end(); ++piZero) {
+    double photonSumPt_insideSignalCone = 0.;
+    double photonSumPt_outsideSignalCone = 0.;
+      int numPhotons = piZero->numberOfDaughters();


mantain indentation

perrotta · 2017-04-24T11:35:38Z

RecoTauTag/RecoTau/plugins/RecoTauBuilderConePlugin.cc

+  PFTau::hadronicDecayMode dm = PFTau::kNull; 
+  unsigned int nPiZeros = 0;
+  const std::vector<RecoTauPiZero>& piZeros = aTau.signalPiZeroCandidates();
+  for(std::vector<RecoTauPiZero>::const_iterator piZero = piZeros.begin();


Range based for loops must be preferred

perrotta · 2017-04-24T11:40:21Z

RecoTauTag/RecoTau/plugins/RecoTauBuilderConePlugin.cc

+    const unsigned int maxPiZeros = PFTau::kOneProngNPiZero;
+    // Determine our track index
+    unsigned int nCharged = pfChs.size();
+    unsigned int trackIndex = (nCharged - 1)*(maxPiZeros + 1);


If pfChs.size()=0 this may lead to an indefinite number (as unsigned int): can it happen?

…pog_technical-developments_coneBasedTaus

cmsbuild · 2017-04-28T23:47:35Z

+1
Tested at: ceb94d1
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-18429/19473/summary.html

cmsbuild · 2017-04-28T23:47:38Z

Comparison job queued.

cmsbuild · 2017-04-29T01:45:51Z

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

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 1676 differences found in the comparisons
DQMHistoTests: Total files compared: 23
DQMHistoTests: Total histograms compared: 1778721
DQMHistoTests: Total failures: 37744
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 1740804
DQMHistoTests: Total skipped: 173
DQMHistoTests: Total Missing objects: 0
Checked 94 log files, 14 edm output root files, 23 DQM output files

davidlange6 · 2017-05-02T07:44:44Z

HLTrigger/Configuration/python/customizeHLTforCMSSW.py

+     for producer in producers_by_type(process, "RecoTauProducer"):
+          if hasattr(producer,'builders'):
+               for pset in producer.builders:
+                    if not hasattr(pset,'minAbsPhotonSumPt_insideSignalCone'):


so this looks to be just a more inefficient version of existsAs... Could this not be replaced by a fillDescriptions instead?

@davidlange6 Yes, in principle fillDescriptions will solve this and similar issues caused by the fact that HLT configuration is dumped from ConfDB which knows on new parameters only when a new release is parsed into it. The thing is that the tau reco code uses quite heavily plugin factories which, at least in my understanding, are quite incompatible with the fillDescriptions mechanism. So mitigation from python cfi files to the fillDescriptions mechanism means rewriting tau code which is a big effort.

davidlange6 · 2017-05-02T10:16:07Z

I'm not sure what prevents the plugins to use fillDescriptions - but in the meanwhile, i think I would then suggest the bad old fashioned existsAs as to avoid yet another loop over all producers in the HLT. but maybe i don't know all the constraints.

…

On May 2, 2017, at 10:49 AM, mbluj ***@***.***> wrote: @mbluj commented on this pull request. In HLTrigger/Configuration/python/customizeHLTforCMSSW.py: > @@ -71,6 +71,18 @@ def customiseFor18330(process): for pset in producer.vertexCollections: if pset.algorithm.value() == "AdaptiveVertexFitter" and not hasattr(pset, "chi2cutoff"): pset.chi2cutoff = cms.double(3.0) + return process + + +# Add new parameters required by RecoTauBuilderConePlugin +def customiseFor18429(process): + for producer in producers_by_type(process, "RecoTauProducer"): + if hasattr(producer,'builders'): + for pset in producer.builders: + if not hasattr(pset,'minAbsPhotonSumPt_insideSignalCone'): @davidlange6 Yes, in principle fillDescriptions will solve this and similar issues caused by the fact that HLT configuration is dumped from ConfDB which knows on new parameters only when a new release is parsed into it. The thing is that the tau reco code uses quite heavily plugin factories which, at least in my understanding, are quite incompatible with the fillDescriptions mechanism. So mitigation from python cfi files to the fillDescriptions mechanism means rewriting tau code which is a big effort. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

mbluj · 2017-05-02T10:36:21Z

On Tue, May 2, 2017 at 12:16 PM, David Lange <notifications@github.com> wrote:

I'm not sure what prevents the plugins to use fillDescriptions

Maybe it is my ignorance, but tau reco has a nested structure with one plugin calling other ones. Depending on configuration the set of nested plugins can be different, so I do not think that there is a way of sensible configuration of them with fillDescriptions.

- but in the meanwhile, i think I would then suggest the bad old fashioned existsAs as to avoid yet another loop over all producers in the HLT. but maybe i don't know all the constraints.

It is what we agree, the existsAs constructions should be removed. But, this particular customization (customiseFor18429) is not about this; it just adds missing tracked parameters to HLT configuration as the configuration does not know nothing about them. It is as it the HLT configuration dumped from ConfDB where configuration templates come from an older release. The customization is temporary and will disappear when configuration in HLT ConfDB are refreshed with newer release. I hope, that this time I was able to explain it more clear than previously.

…

> On May 2, 2017, at 10:49 AM, mbluj ***@***.***> wrote: > > @mbluj commented on this pull request. > > In HLTrigger/Configuration/python/customizeHLTforCMSSW.py: > > > @@ -71,6 +71,18 @@ def customiseFor18330(process): > for pset in producer.vertexCollections: > if pset.algorithm.value() == "AdaptiveVertexFitter" and not hasattr(pset, "chi2cutoff"): > pset.chi2cutoff = cms.double(3.0) > + return process > + > + > +# Add new parameters required by RecoTauBuilderConePlugin > +def customiseFor18429(process): > + for producer in producers_by_type(process, "RecoTauProducer"): > + if hasattr(producer,'builders'): > + for pset in producer.builders: > + if not hasattr(pset,'minAbsPhotonSumPt_insideSignalCone'): > > @davidlange6 Yes, in principle fillDescriptions will solve this and similar issues caused by the fact that HLT configuration is dumped from ConfDB which knows on new parameters only when a new release is parsed into it. The thing is that the tau reco code uses quite heavily plugin factories which, at least in my understanding, are quite incompatible with the fillDescriptions mechanism. So mitigation from python cfi files to the fillDescriptions mechanism means rewriting tau code which is a big effort. >

fwyzard · 2017-05-02T11:07:37Z

On 2 May 2017 at 12:36, mbluj ***@***.***> wrote: > I'm not sure what prevents the plugins to use fillDescriptions Maybe it is my ignorance, but tau reco has a nested structure with one plugin calling other ones. Depending on configuration the set of nested plugins can be different, so I do not think that there is a way of sensible configuration of them with fillDescriptions.

The required functionality is in my wish list of CMSSW features since few years... .A

…

-- Strategy is a system of expedients. Generalfeldmarschall Helmuth Karl Bernhard Graf von Moltke

davidlange6 · 2017-05-02T11:44:24Z

any idea what functionality is actually missing? Some "CMSSW" (eg, framework) feature or just that developers need to do something? I would have thought that any plugin could define a static function that gets called by the module level fillDescriptions to hierarchically define a pset. I've not thought about issues such as dependencies however. Has anyone tried an implementation that didn't work out?

…

On May 2, 2017, at 1:07 PM, Andrea Bocci ***@***.***> wrote: On 2 May 2017 at 12:36, mbluj ***@***.***> wrote: > > I'm not sure what prevents the plugins to use fillDescriptions > > Maybe it is my ignorance, but tau reco has a nested structure with one > plugin calling other ones. Depending on configuration the set of nested > plugins can be different, so I do not think that there is a way of sensible > configuration of them with fillDescriptions. > The required functionality is in my wish list of CMSSW features since few years... .A -- Strategy is a system of expedients. Generalfeldmarschall Helmuth Karl Bernhard Graf von Moltke — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

makortel · 2017-05-02T12:10:52Z

@davidlange6 See e.g. #12517 (comment), and especially https://hypernews.cern.ch/HyperNews/CMS/get/edmFramework/3446/1.html.

(personally I've given up the hope in favour of moving helper plugins to ED/ESProducts, depending if they need edm::Event or not, even if this approach is rather laborious)

davidlange6 · 2017-05-02T12:25:58Z

thanks.

…

On May 2, 2017, at 2:10 PM, Matti Kortelainen ***@***.***> wrote: @davidlange6 See e.g. #12517 (comment), and especially https://hypernews.cern.ch/HyperNews/CMS/get/edmFramework/3446/1.html. (personally I've given up the hope in favour of moving helper plugins to ED/ESProducts, depending if they need edm::Event or not, even if this approach is rather laborious) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

davidlange6 · 2017-05-04T08:50:02Z

it is what we agree, the existsAs constructions should be removed. But,
this particular customization (customiseFor18429) is not about this; it

to be clear I said the opposite - we removed an ugly, but fast/simple construct (existsas) in favor of an ugly and slow construct (for modules in hlt loop....).

fwyzard · 2017-05-04T21:10:33Z

On 4 May 2017 at 10:50, David Lange ***@***.***> wrote: to be clear I said the opposite - we removed an ugly, but fast/simple construct (existsas) in favor of an ugly and slow construct (for modules in hlt loop....).

The "ugly, but fast/simple construct (existsas)" happened at runtime, and was not tracked in the provenance. The "ugly and slow construct (for modules in hlt loop....)" can be made to happen only once by dumping the configuration (which I think crab and other tools do), and not at all once the new configuration is parsed and made available. In addition the provenance sees the expanded configuration with all the parameters. .A

…

-- Strategy is a system of expedients. Generalfeldmarschall Helmuth Karl Bernhard Graf von Moltke

Martin-Grunewald · 2017-05-05T06:36:05Z

Since this PR has become a 92X PR, please make an explicit 91X PR as well!
Thanks!

davidlange6 · 2017-05-05T07:24:26Z

+1

mbluj · 2017-05-05T08:37:10Z

Since this PR has become a 92X PR, please make an explicit 91X PR as well! Thanks!

Done: #18581 <https://www.avast.com/sig-email?utm_medium=email&utm_source=link&utm_campaign=sig-email&utm_content=webmail> Virus-free. www.avast.com <https://www.avast.com/sig-email?utm_medium=email&utm_source=link&utm_campaign=sig-email&utm_content=webmail> <#DAB4FAD8-2DD7-40BB-A1B8-4E2AA1F9FDF2>

mbluj and others added 4 commits April 8, 2017 18:04

Merged refs/pull/18243/head from repository cms-sw

e3bf6ec

changes to coneTauProducer to fill missing quantities

21003b0

Merge pull request #44 from mbluj/coneTaus_91X

199f427

Modified Cone-based Tau Builder

Merge branch 'from-CMSSW_9_1_X_2017-04-13-2300' into CMSSW_9_1_X_tau-…

7c6e96e

…pog_technical-developments_coneBasedTaus

cmsbuild added this to the Next CMSSW_9_1_X milestone Apr 20, 2017

cmsbuild added comparison-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Apr 20, 2017

cmsbuild added tests-started and removed tests-pending labels Apr 20, 2017

cmsbuild added tests-approved and removed tests-started labels Apr 21, 2017

cmsbuild added comparison-available and removed comparison-pending labels Apr 21, 2017

roger-wolf mentioned this pull request Apr 22, 2017

Modification of cone based tau algorithm to furbish it as fallback solution for the HLT for upcoming data taking (90X version) #18441

Closed

perrotta reviewed Apr 24, 2017

View reviewed changes

roger-wolf added 2 commits April 24, 2017 22:53

Merge branch 'from-CMSSW_9_1_X_2017-04-24-1100' into CMSSW_9_1_X_tau-…

7e7b6ec

…pog_technical-developments_coneBasedTaus

following requests by RECO

c425d8e

cmsbuild added comparison-pending tests-pending and removed comparison-available tests-approved labels Apr 24, 2017

cmsbuild added comparison-pending tests-pending tests-started and removed tests-pending labels Apr 28, 2017

cmsbuild added tests-approved and removed tests-started labels Apr 28, 2017

cmsbuild added comparison-available and removed comparison-pending labels Apr 29, 2017

davidlange6 reviewed May 2, 2017

View reviewed changes

smuzaffar modified the milestones: CMSSW_9_2_X, CMSSW_9_1_X May 4, 2017

cmsbuild added orp-approved and removed orp-pending labels May 5, 2017

cmsbuild merged commit abdea55 into cms-sw:master May 5, 2017

mbluj mentioned this pull request May 5, 2017

Modification of cone based tau algorithm to furbish it as fallback solution for the HLT for upcoming data taking (91X) #18581

Merged

mbluj deleted the CMSSW_9_1_X_tau-pog_technical-developments_coneBasedTaus branch October 10, 2023 10:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modification of cone based tau algorithm to furbish it as fallback solution for the HLT for upcoming data taking #18429

Modification of cone based tau algorithm to furbish it as fallback solution for the HLT for upcoming data taking #18429

roger-wolf commented Apr 20, 2017

cmsbuild commented Apr 20, 2017 •

edited by smuzaffar

slava77 commented Apr 20, 2017

cmsbuild commented Apr 20, 2017 •

edited

cmsbuild commented Apr 21, 2017

cmsbuild commented Apr 21, 2017

cmsbuild commented Apr 21, 2017

perrotta Apr 24, 2017

perrotta Apr 24, 2017

perrotta Apr 24, 2017

perrotta Apr 24, 2017

cmsbuild commented Apr 28, 2017

cmsbuild commented Apr 28, 2017

cmsbuild commented Apr 29, 2017

davidlange6 May 2, 2017

mbluj May 2, 2017

davidlange6 commented May 2, 2017 via email

mbluj commented May 2, 2017 via email

fwyzard commented May 2, 2017 via email

davidlange6 commented May 2, 2017 via email

makortel commented May 2, 2017

davidlange6 commented May 2, 2017 via email

davidlange6 commented May 4, 2017

fwyzard commented May 4, 2017 via email

Martin-Grunewald commented May 5, 2017

davidlange6 commented May 5, 2017

mbluj commented May 5, 2017 via email

Modification of cone based tau algorithm to furbish it as fallback solution for the HLT for upcoming data taking #18429

Modification of cone based tau algorithm to furbish it as fallback solution for the HLT for upcoming data taking #18429

Conversation

roger-wolf commented Apr 20, 2017

cmsbuild commented Apr 20, 2017 • edited by smuzaffar

slava77 commented Apr 20, 2017

cmsbuild commented Apr 20, 2017 • edited

cmsbuild commented Apr 21, 2017

cmsbuild commented Apr 21, 2017

cmsbuild commented Apr 21, 2017

perrotta Apr 24, 2017

Choose a reason for hiding this comment

perrotta Apr 24, 2017

Choose a reason for hiding this comment

perrotta Apr 24, 2017

Choose a reason for hiding this comment

perrotta Apr 24, 2017

Choose a reason for hiding this comment

cmsbuild commented Apr 28, 2017

cmsbuild commented Apr 28, 2017

cmsbuild commented Apr 29, 2017

davidlange6 May 2, 2017

Choose a reason for hiding this comment

mbluj May 2, 2017

Choose a reason for hiding this comment

davidlange6 commented May 2, 2017 via email

mbluj commented May 2, 2017 via email

fwyzard commented May 2, 2017 via email

davidlange6 commented May 2, 2017 via email

makortel commented May 2, 2017

davidlange6 commented May 2, 2017 via email

davidlange6 commented May 4, 2017

fwyzard commented May 4, 2017 via email

Martin-Grunewald commented May 5, 2017

davidlange6 commented May 5, 2017

mbluj commented May 5, 2017 via email

cmsbuild commented Apr 20, 2017 •

edited by smuzaffar

cmsbuild commented Apr 20, 2017 •

edited