Refactor tau cluster variable computation #24382

mbluj · 2018-08-24T10:26:06Z

Refactor tau cluster variable computation: put MVA calculation for pat::Taus into non-class method.

Description from developer (Jan Steggemann @steggema):
With the MVA calculation a non-class method, it can directly be used from FWlite so that the tau MVA isolation can also be evaluated from FWlite. Code to do this will be merged separately via Heppy. This is meant to be a preceding PR that factors out code that affects Taus, and in particular code run in the Tau RECO sequence. No changes of any tau reco quantities are expected.

…t::Taus into non-class method

Refactor tau cluster variable computation

cmsbuild · 2018-08-24T10:26:24Z

The code-checks are being triggered in jenkins.

cmsbuild · 2018-08-24T10:28:40Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-24382/6151

cmsbuild · 2018-08-24T10:28:59Z

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

It involves the following packages:

RecoTauTag/RecoTau

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

cms-bot commands are listed here

perrotta · 2018-08-24T11:03:06Z

please test

cmsbuild · 2018-08-24T11:03:24Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/30036/console Started: 2018/08/24 13:05

cmsbuild · 2018-08-24T13:58:19Z

+1
Tested at: 23da7ae
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-24382/30036/summary.html

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

cmsbuild · 2018-08-24T13:58:22Z

Comparison job queued.

cmsbuild · 2018-08-24T15:46:20Z

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

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 31
DQMHistoTests: Total histograms compared: 2987459
DQMHistoTests: Total failures: 2
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2987267
DQMHistoTests: Total skipped: 190
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 30 files compared)
Checked 129 log files, 14 edm output root files, 31 DQM output files

slava77 · 2018-08-28T19:49:53Z

RecoTauTag/RecoTau/interface/PFRecoTauClusterVariables.h

-      }
-      return n_photons;
-    }
+namespace reco { namespace tau { namespace mva {


IIUC, this is a new namespace.
Is it really needed and are the methods MVA-specific?
We have reco::tau:disc and reco::tau::helpers, which could be used instead.
IMHO, reco::tau is enough.

slava77 · 2018-08-28T19:52:12Z

RecoTauTag/RecoTau/interface/PFRecoTauClusterVariables.h

-    }
+namespace reco { namespace tau { namespace mva {
+  /// return chi2 of the leading track ==> deprecated? <==
+  float tau_leadTrackChi2(const reco::PFTau& tau);


is the tau_ prefix needed for the methods? everything is in the tau namespace. So, perhaps no prefix is OK.
I see that some of the methods do not have a tau_ prefix.

slava77 · 2018-08-28T20:01:39Z

RecoTauTag/RecoTau/src/PFRecoTauClusterVariables.cc

+    }
+    return 0.;
+  }
+  float pt_weighted_dx(const pat::Tau& tau, int mode, int var, int decaymode) {


the implementation is identical to pt_weighted_dx(const PFTau& tau. It looks like a good candidate for a template

here is one option
2169a12

is this really unreadable?
I think that the danger of missing an update in either of the two duplicated places is worse than slight increase in complexity.

slava77 · 2018-08-28T20:02:30Z

RecoTauTag/RecoTau/interface/PFRecoTauClusterVariables.h

+  /// return sum of pt weighted values of dr relative to tau candidate for all pf photon candidates,
+  /// which are associated to signal
+  float tau_pt_weighted_dr_signal(const reco::PFTau& tau, int dm);
+  float tau_pt_weighted_dr_signal(const pat::Tau& tau, int dm);


these could stay inlined (perhaps with an explicit inline qualifier).
Same for tau_pt_weighted_deta_strip, tau_pt_weighted_dphi_strip, and tau_pt_weighted_dr_iso.

slava77 · 2018-08-28T20:10:11Z

RecoTauTag/RecoTau/interface/PFRecoTauClusterVariables.h

-    }      
-};
+  enum { kOldDMwoLT, kOldDMwLT, kNewDMwoLT, kNewDMwLT, kDBoldDMwLT, kDBnewDMwLT, kPWoldDMwLT, kPWnewDMwLT, kDBoldDMwLTwGJ, kDBnewDMwLTwGJ };
+  bool fillMVAInputs(float* mvaInput, const pat::Tau& tau, int mvaOpt, const std::string nameCharged, const std::string nameNeutral, const std::string namePu, const std::string nameOutside, const std::string nameFootprint);


please add a newline for readability.
const std::string&

slava77 · 2018-08-28T20:17:12Z

RecoTauTag/RecoTau/src/PFRecoTauClusterVariables.cc

+
+      // --- The following 5 variables differ slightly between AOD & MiniAOD
+      //     because they are recomputed using packedCandidates saved in the tau
+      float nPhoton = (float)reco::tau::mva::tau_n_photons_total(tau);


the extra cast on the rhs is not needed

slava77 · 2018-08-28T20:19:56Z

RecoTauTag/RecoTau/src/PFRecoTauClusterVariables.cc

+    return false;
+  }
+
+}}} // namespaces


please terminate the file with a newline

slava77 · 2018-08-28T20:29:35Z

RecoTauTag/RecoTau/plugins/PFRecoTauDiscriminationByMVAIsolationRun2.cc

@@ -161,7 +161,6 @@ class PFRecoTauDiscriminationByMVAIsolationRun2 : public PFTauDiscriminationProd
  std::unique_ptr<PFTauDiscriminator> category_output_;


the enums defined on L141 are now somewhat redundant with what's defined in the included PFRecoTauClusterVariables.h.
There is no obvious conflict here due to still fully localized use of these values in mvaOpt_.
So, cleanup is not required, but could still be better.

slava77 · 2018-08-28T20:33:06Z

RecoTauTag/RecoTau/src/PFRecoTauClusterVariables.cc

+    float HcalEnInSignalPFCands = 0;
+    typedef std::vector <reco::PFCandidatePtr>::iterator constituents_iterator;
+    for(constituents_iterator it=constsignal.begin(); it != constsignal.end(); ++it) {
+      reco::PFCandidatePtr & icand = *it;


some modernization of the code can be done, since we have an opportunity with this refactoring:

range for loops for (auto const& icand : constsignal)

Making the local variable names conform to the CMS style of starting with lower case for locals and data members/methods would be nice as well.

slava77 · 2018-08-28T20:36:20Z

RecoTauTag/RecoTau/src/PFRecoTauClusterVariables.cc

+    return n_photons;
+  }  
+
+  bool fillMVAInputs(float* mvaInput, const pat::Tau& tau, int mvaOpt, const std::string nameCharged, const std::string   nameNeutral, const std::string namePu, const std::string nameOutside, const std::string nameFootprint) 


should I take this replacement of the implementation of PATTauDiscriminationByMVAIsolationRun2 as a move to the new standard single supported MVA?
If the other variants are expected to remain supported, then a simple fillMVAInputs method name here is not enough. In this case please make a distinction with some specification in the function name.

We are a bit confused by the comment: PATTauDiscriminationByMVAIsolationRun2 plugin is the single implementation of MVA-based isolation tau-Ids with the fillMVAInputs method having itself all the different MVAIso variants included. Could you please clarify what you exactly mean?

Regardless, we can rename the method to fillIsoMVARun2Inputs (Iso and Run2 added) to better distinguish from other possible MVA-based tau-ids, e.g. anti electron ones, which could in principle use the cluster variables.

currently there are different tau MVAs in the release. So, it seems unnatural to single out one and fill it with a generically named function.
To me this either means that this PR is the first step to eliminate/subsume other MVA filling implementations or the method is named improperly.

I think I'm still confused - the method as implemented works for all tau MVAs by means of the "mvaOpt" argument.

Concerning dropping the possibility to recalculate previous versions of the MVA from the tau code, I'm in principle in favour of this since it will make the code simpler, but it's not the focus of this PR.

OK, thank you for clarification.

Just for notice, there are the following MVA-based tau-Ids and related plugins:

MVA-based isolation:

PFRecoTauDiscriminationByMVAIsolation2.cc for Run-1

PFRecoTauDiscriminationByMVAIsolationRun2.cc for Run-2 with its counterpart for pat::Taus called PATTauDiscriminationByMVAIsolationRun2.cc

e->tauh fakes veto:

PFRecoTauDiscriminationAgainstElectronMVA5.cc for Run-1

PFRecoTauDiscriminationAgainstElectronMVA6.cc for Run-2 with its counterpart for pat::Taus called PATTauDiscriminationAgainstElectronMVA6.cc

mu->tauh fakes veto:

PFRecoTauDiscriminationAgainstMuonMVA.cc

The old Run-1 plugins can be removed at some point, but it is beyond a scope of this PR.

To avoid confusion we will also rename the method to fillIsoMVARun2Inputs to indicate that it is used for this particular MVA-based tau-Id.

Address code review comments, including modernisation of code

cmsbuild · 2018-09-03T15:38:08Z

The code-checks are being triggered in jenkins.

cmsbuild · 2018-09-03T15:39:50Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-24382/6279

cmsbuild · 2018-09-03T15:40:08Z

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

slava77 · 2018-09-03T15:54:19Z

@cmsbuild please test

cmsbuild · 2018-09-03T15:54:35Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/30226/console Started: 2018/09/03 17:55

cmsbuild · 2018-09-03T18:23:27Z

+1
Tested at: 79da684
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-24382/30226/summary.html

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:
79da684
bc2e5cc
43fbbdd
1ad3da6
23da7ae
You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-24382/30226/git-log-recent-commits
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-24382/30226/git-merge-result

cmsbuild · 2018-09-03T18:23:30Z

Comparison job queued.

cmsbuild · 2018-09-03T19:48:00Z

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

Comparison Summary:

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

slava77 · 2018-09-04T22:26:47Z

+1

for #24382 79da684

code changes are in line with the PR description and the follow up review.
jenkins tests pass and comparisons with the baseline show no differences

cmsbuild · 2018-09-04T22:27:04Z

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 · 2018-09-06T17:29:46Z

+1

steggema and others added 2 commits August 23, 2018 13:49

Refactor tau cluster variable computation; put MVA calculation for pa…

589eefb

…t::Taus into non-class method

Merge pull request #80 from steggema/TauClusterVarRefactor_10_3_X

23da7ae

Refactor tau cluster variable computation

cmsbuild added this to the CMSSW_10_3_X milestone Aug 24, 2018

cmsbuild added code-checks-pending comparison-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Aug 24, 2018

cmsbuild added code-checks-approved and removed code-checks-pending labels Aug 24, 2018

cmsbuild added tests-started and removed tests-pending labels Aug 24, 2018

cmsbuild added tests-approved and removed tests-started labels Aug 24, 2018

cmsbuild added comparison-available and removed comparison-pending labels Aug 24, 2018

slava77 reviewed Aug 28, 2018

View reviewed changes

steggema and others added 3 commits August 30, 2018 17:01

Address code review comments, including modernisation of code

eef7b80

Create template function to avoid code duplication

dad7231

Merge pull request #82 from steggema/TauClusterVarRefactor_10_3_X

1ad3da6

Address code review comments, including modernisation of code

cmsbuild removed the code-checks-approved label Aug 30, 2018

cmsbuild added code-checks-approved and removed code-checks-pending labels Sep 3, 2018

cmsbuild added tests-started and removed tests-pending labels Sep 3, 2018

cmsbuild added tests-approved and removed tests-started labels Sep 3, 2018

cmsbuild added comparison-available and removed comparison-pending labels Sep 3, 2018

cmsbuild added fully-signed reconstruction-approved and removed pending-signatures reconstruction-pending labels Sep 4, 2018

cmsbuild added orp-approved and removed orp-pending labels Sep 6, 2018

cmsbuild merged commit e123fa2 into cms-sw:master Sep 6, 2018

mbluj mentioned this pull request Sep 7, 2018

Tau reconstruction based on miniAOD event content #22594

Merged

mbluj mentioned this pull request Dec 3, 2018

DNN-based Tau-Id discrimians (102X) #25386

Merged

mbluj mentioned this pull request Jun 11, 2019

Update of DeepTauID to ver. 2017v2 (backport to 10_2_X) #27148

Merged

mbluj deleted the CMSSW_10_3_X_tau_pog_TauClusterVarRefactor branch October 10, 2023 10:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor tau cluster variable computation #24382

Refactor tau cluster variable computation #24382

mbluj commented Aug 24, 2018

cmsbuild commented Aug 24, 2018

cmsbuild commented Aug 24, 2018

cmsbuild commented Aug 24, 2018

perrotta commented Aug 24, 2018

cmsbuild commented Aug 24, 2018 •

edited

cmsbuild commented Aug 24, 2018

cmsbuild commented Aug 24, 2018

cmsbuild commented Aug 24, 2018

slava77 Aug 28, 2018

slava77 Aug 28, 2018

slava77 Aug 28, 2018

slava77 Aug 31, 2018

slava77 Aug 28, 2018

slava77 Aug 28, 2018

slava77 Aug 28, 2018

slava77 Aug 28, 2018

slava77 Aug 28, 2018

slava77 Aug 28, 2018

slava77 Aug 28, 2018

mbluj Aug 29, 2018

slava77 Aug 29, 2018

steggema Aug 29, 2018

mbluj Aug 29, 2018

cmsbuild commented Sep 3, 2018

cmsbuild commented Sep 3, 2018

cmsbuild commented Sep 3, 2018

slava77 commented Sep 3, 2018

cmsbuild commented Sep 3, 2018 •

edited

cmsbuild commented Sep 3, 2018

cmsbuild commented Sep 3, 2018

cmsbuild commented Sep 3, 2018

slava77 commented Sep 4, 2018

cmsbuild commented Sep 4, 2018

fabiocos commented Sep 6, 2018

		@@ -161,7 +161,6 @@ class PFRecoTauDiscriminationByMVAIsolationRun2 : public PFTauDiscriminationProd
		std::unique_ptr<PFTauDiscriminator> category_output_;

Refactor tau cluster variable computation #24382

Refactor tau cluster variable computation #24382

Conversation

mbluj commented Aug 24, 2018

cmsbuild commented Aug 24, 2018

cmsbuild commented Aug 24, 2018

cmsbuild commented Aug 24, 2018

perrotta commented Aug 24, 2018

cmsbuild commented Aug 24, 2018 • edited

cmsbuild commented Aug 24, 2018

cmsbuild commented Aug 24, 2018

cmsbuild commented Aug 24, 2018

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

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

Choose a reason for hiding this comment

cmsbuild commented Sep 3, 2018

cmsbuild commented Sep 3, 2018

cmsbuild commented Sep 3, 2018

slava77 commented Sep 3, 2018

cmsbuild commented Sep 3, 2018 • edited

cmsbuild commented Sep 3, 2018

cmsbuild commented Sep 3, 2018

cmsbuild commented Sep 3, 2018

slava77 commented Sep 4, 2018

cmsbuild commented Sep 4, 2018

fabiocos commented Sep 6, 2018

cmsbuild commented Aug 24, 2018 •

edited

cmsbuild commented Sep 3, 2018 •

edited