Fix charged and neutral candidate converters for reco::Jets #35922

SWuchterl · 2021-10-31T14:42:48Z

PR description:

This PR allows the evaluation of DeepJet on reco::Jets, which to my understanding should have worked before the latest change. With the current logic, only pat::Jets are accepted.
The fix is needed to use DeepJet at HLT. For the reco:: case, features needed for DeepDoubleX are filled with the same dummy variables as in the case of no subjets.

PR validation:

Code compiles.
40 39 38 29 18 4 1 1 1 tests passed, 0 0 0 0 0 0 0 0 0 failed
Also, tested on data and MC samples, running a modified HLT menu with DeepJet instead of DeepCSV.

if this PR is a backport please specify the original PR and why you need to backport that PR:

N/A

cmsbuild · 2021-10-31T14:50:32Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35922/26327

This PR adds an extra 16KB to repository

cmsbuild · 2021-10-31T14:50:56Z

A new Pull Request was created by @SWuchterl (Sebastian Wuchterl) for master.

It involves the following packages:

RecoBTag/FeatureTools (reconstruction)

@jpata, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@AlexDeMoor, @emilbols, @JyothsnaKomaragiri, @hqucms, @andrzejnovak, @demuller this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

slava77 · 2021-10-31T15:02:04Z

@cmsbuild please test

cmsbuild · 2021-10-31T16:16:12Z

-1

Failed Tests: RelVals RelVals-INPUT AddOn
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-4a1543/20121/summary.html
COMMIT: 082f9db
CMSSW: CMSSW_12_2_X_2021-10-31-0000/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/35922/20121/install.sh to create a dev area with all the needed externals and cmssw changes.

RelVals

135.4

----- Begin Fatal Exception 31-Oct-2021 16:23:06 CET-----------------------
An exception of category 'EventSetupConflict' occurred while
   [0] Calling beginJob
Exception Message:
two EventSetup Sources want to deliver type="PPSAssociationCuts" label=""
 from record PPSAssociationCutsRcd. The two providers are 
1) type="PPSAssociationCutsESSource" label="ppsAssociationCutsESSource"
2) type="PoolDBESSource" label="GlobalTag"
Please either
   remove one of these Sources
   or find a way of configuring one of them so it does not deliver this data
   or use an es_prefer statement in the configuration to choose one.
----- End Fatal Exception -------------------------------------------------

136.8311

----- Begin Fatal Exception 31-Oct-2021 16:23:36 CET-----------------------
An exception of category 'EventSetupConflict' occurred while
   [0] Calling beginJob
Exception Message:
two EventSetup Sources want to deliver type="PPSAssociationCuts" label=""
 from record PPSAssociationCutsRcd. The two providers are 
1) type="PPSAssociationCutsESSource" label="ppsAssociationCutsESSource"
2) type="PoolDBESSource" label="GlobalTag"
Please either
   remove one of these Sources
   or find a way of configuring one of them so it does not deliver this data
   or use an es_prefer statement in the configuration to choose one.
----- End Fatal Exception -------------------------------------------------

136.7611

----- Begin Fatal Exception 31-Oct-2021 16:23:37 CET-----------------------
An exception of category 'EventSetupConflict' occurred while
   [0] Calling beginJob
Exception Message:
two EventSetup Sources want to deliver type="PPSAssociationCuts" label=""
 from record PPSAssociationCutsRcd. The two providers are 
1) type="PPSAssociationCutsESSource" label="ppsAssociationCutsESSource"
2) type="PoolDBESSource" label="GlobalTag"
Please either
   remove one of these Sources
   or find a way of configuring one of them so it does not deliver this data
   or use an es_prefer statement in the configuration to choose one.
----- End Fatal Exception -------------------------------------------------

Expand to see more relval errors ...

RelVals-INPUT

4.64.6_MinimumBias2010A+MinimumBias2010A+RECOSKIMALCA+HARVESTDR1/step2_MinimumBias2010A+MinimumBias2010A+RECOSKIMALCA+HARVESTDR1.log
136.72411136.72411_RunJetHT2016B_reminiaodUL+RunJetHT2016B_reminiaodUL+REMINIAOD_data2016UL_HIPM+HARVESTDR2_REMINIAOD_data2016UL_HIPM/step2_RunJetHT2016B_reminiaodUL+RunJetHT2016B_reminiaodUL+REMINIAOD_data2016UL_HIPM+HARVESTDR2_REMINIAOD_data2016UL_HIPM.log
136.72412136.72412_RunJetHT2016B_reminiaodUL+RunJetHT2016B_reminiaodUL+REMININANO_data2016UL_HIPM+HARVESTDR2_REMININANO_data2016UL_HIPM/step2_RunJetHT2016B_reminiaodUL+RunJetHT2016B_reminiaodUL+REMININANO_data2016UL_HIPM+HARVESTDR2_REMININANO_data2016UL_HIPM.log

Expand to see more relval errors ...

AddOn Tests

fastsim1

----- Begin Fatal Exception 31-Oct-2021 16:22:19 CET-----------------------
An exception of category 'EventSetupConflict' occurred while
   [0] Calling beginJob
Exception Message:
two EventSetup Sources want to deliver type="PPSAssociationCuts" label=""
 from record PPSAssociationCutsRcd. The two providers are 
1) type="PPSAssociationCutsESSource" label="ppsAssociationCutsESSource"
2) type="PoolDBESSource" label="GlobalTag"
Please either
   remove one of these Sources
   or find a way of configuring one of them so it does not deliver this data
   or use an es_prefer statement in the configuration to choose one.
----- End Fatal Exception -------------------------------------------------

fastsim2

----- Begin Fatal Exception 31-Oct-2021 16:22:28 CET-----------------------
An exception of category 'EventSetupConflict' occurred while
   [0] Calling beginJob
Exception Message:
two EventSetup Sources want to deliver type="PPSAssociationCuts" label=""
 from record PPSAssociationCutsRcd. The two providers are 
1) type="PPSAssociationCutsESSource" label="ppsAssociationCutsESSource"
2) type="PoolDBESSource" label="GlobalTag"
Please either
   remove one of these Sources
   or find a way of configuring one of them so it does not deliver this data
   or use an es_prefer statement in the configuration to choose one.
----- End Fatal Exception -------------------------------------------------

hlt_mc_Fake1

----- Begin Fatal Exception 31-Oct-2021 16:26:32 CET-----------------------
An exception of category 'EventSetupConflict' occurred while
   [0] Calling beginJob
Exception Message:
two EventSetup Sources want to deliver type="PPSAssociationCuts" label=""
 from record PPSAssociationCutsRcd. The two providers are 
1) type="PPSAssociationCutsESSource" label="ppsAssociationCutsESSource"
2) type="PoolDBESSource" label="GlobalTag"
Please either
   remove one of these Sources
   or find a way of configuring one of them so it does not deliver this data
   or use an es_prefer statement in the configuration to choose one.
----- End Fatal Exception -------------------------------------------------

Expand to see more addon errors ...

slava77 · 2021-10-31T16:28:38Z

The IB is broken.

tests will need to be restarted after the new IB shows up anyways to get appropriate comparisons

VinInn · 2021-11-01T11:31:10Z

RecoBTag/FeatureTools/interface/ChargedCandidateConverter.h

+      if (patJet->nSubjetCollections() > 0) {
+        auto subjets = patJet->subjets();
+        // sort by pt
+        std::sort(subjets.begin(), subjets.end(), [](const edm::Ptr<pat::Jet>& p1, const edm::Ptr<pat::Jet>& p2) {


no need for a full sorting here.

Those pieces of the code were already there, I just inverted the logic of the if statement in the beginning. From the diff it seems different, I know. But of course, I can change it as suggested.
(Also applies for the second comment below)
Just for clarification, do you suggest removing the line entirely because the collection should already be sorted?

stil this code is very sub-optimal

@SWuchterl I understand you only need the first and second jet by pt, so you don't need to do a full sorting. This could be a straightforward improvement for both the new and original code. I'm not sure you can safely assume that the collection is sorted.

We can also run a profiling on this PR to check if this is a measurable/detectable issue.

VinInn · 2021-11-01T11:31:51Z

RecoBTag/FeatureTools/interface/ChargedCandidateConverter.h

+        std::sort(subjets.begin(), subjets.end(), [](const edm::Ptr<pat::Jet>& p1, const edm::Ptr<pat::Jet>& p2) {
+          return p1->pt() > p2->pt();
+        });
+        c_pf_features.drsubjet1 = !subjets.empty() ? reco::deltaR(*c_pf, *subjets.at(0)) : -1;


use operator[] not at

VinInn · 2021-11-01T11:32:26Z

RecoBTag/FeatureTools/interface/NeutralCandidateConverter.h

-      });
-      n_pf_features.drsubjet1 = !subjets.empty() ? reco::deltaR(*n_pf, *subjets.at(0)) : -1;
-      n_pf_features.drsubjet2 = subjets.size() > 1 ? reco::deltaR(*n_pf, *subjets.at(1)) : -1;
+    if (patJet) {


why all this code duplication?

Could you please clarify the comment about what code exactly is duplicated in this file?

the code in this file looks identical to the one in the previous one.

@SWuchterl for clarification, the duplication is between ChargedCandidateConverter.h and NeutralCandidateConverter.h. The new code could be a simple utility function defined once, and called from both files.

The first file is about Charged candidates and this one is about the Neutral ones. Some elements look similar indeed but they define 2 different inputs used by the neural network.

slava77 · 2021-11-01T12:36:23Z

@cmsbuild please test

cmsbuild · 2021-11-01T16:10:33Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-4a1543/20153/summary.html
COMMIT: 082f9db
CMSSW: CMSSW_12_2_X_2021-10-31-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/35922/20153/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 4 differences found in the comparisons
DQMHistoTests: Total files compared: 42
DQMHistoTests: Total histograms compared: 2901890
DQMHistoTests: Total failures: 1
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2901867
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 41 files compared)
Checked 177 log files, 37 edm output root files, 42 DQM output files
TriggerResults: no differences found

jpata

There is some duplication and a possible code optimization that was brought up by Vincenzo that could be addressed. I don't see any reco changes - good.
After changes are implemented, we could run another round of tests with profiling.

jpata · 2021-11-02T13:25:27Z

RecoBTag/FeatureTools/interface/ChargedCandidateConverter.h

+      if (patJet->nSubjetCollections() > 0) {
+        auto subjets = patJet->subjets();
+        // sort by pt
+        std::sort(subjets.begin(), subjets.end(), [](const edm::Ptr<pat::Jet>& p1, const edm::Ptr<pat::Jet>& p2) {


@SWuchterl I understand you only need the first and second jet by pt, so you don't need to do a full sorting. This could be a straightforward improvement for both the new and original code. I'm not sure you can safely assume that the collection is sorted.

We can also run a profiling on this PR to check if this is a measurable/detectable issue.

jpata · 2021-11-02T13:27:37Z

RecoBTag/FeatureTools/interface/NeutralCandidateConverter.h

-      });
-      n_pf_features.drsubjet1 = !subjets.empty() ? reco::deltaR(*n_pf, *subjets.at(0)) : -1;
-      n_pf_features.drsubjet2 = subjets.size() > 1 ? reco::deltaR(*n_pf, *subjets.at(1)) : -1;
+    if (patJet) {


@SWuchterl for clarification, the duplication is between ChargedCandidateConverter.h and NeutralCandidateConverter.h. The new code could be a simple utility function defined once, and called from both files.

cmsbuild · 2021-11-09T09:25:30Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35922/26512

This PR adds an extra 16KB to repository

cmsbuild · 2021-11-09T09:25:51Z

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

jpata · 2021-11-09T15:43:13Z

@cmsbuild please test

cmsbuild · 2021-11-09T17:17:35Z

-1

Failed Tests: RelVals
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-4a1543/20392/summary.html
COMMIT: 758843c
CMSSW: CMSSW_12_2_X_2021-11-08-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/35922/20392/install.sh to create a dev area with all the needed externals and cmssw changes.

RelVals

11634.911

----- Begin Fatal Exception 09-Nov-2021 17:25:20 CET-----------------------
An exception of category 'PluginNotFound' occurred while
   [0] Constructing the EventProcessor
Exception Message:
Unable to find plugin 'EcalBarrelGeometryEPdd4hep' in category 'CMS EDM Framework ParameterSet Description'. Please check spelling of name.
----- End Fatal Exception -------------------------------------------------

slava77 · 2021-11-09T23:38:45Z

@cmsbuild please test

cmsbuild · 2021-11-10T02:52:43Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-4a1543/20416/summary.html
COMMIT: 758843c
CMSSW: CMSSW_12_2_X_2021-11-09-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/35922/20416/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 4 differences found in the comparisons
DQMHistoTests: Total files compared: 42
DQMHistoTests: Total histograms compared: 2901890
DQMHistoTests: Total failures: 6
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2901862
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 41 files compared)
Checked 177 log files, 37 edm output root files, 42 DQM output files
TriggerResults: no differences found

jpata · 2021-11-11T11:06:11Z

+reconstruction

fix for using DeepJet at HLT by setting the subjet features.
no reco changes expected or observed

cmsbuild · 2021-11-11T11:06:32Z

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. @perrotta, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

qliphy · 2021-11-11T11:51:46Z

+1

allow reco::Jets in candidate converters

082f9db

cmsbuild added this to the CMSSW_12_2_X milestone Oct 31, 2021

cmsbuild added code-checks-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Oct 31, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Oct 31, 2021

cmsbuild added tests-started and removed tests-pending labels Oct 31, 2021

cmsbuild added tests-rejected and removed tests-started labels Oct 31, 2021

VinInn reviewed Nov 1, 2021

View reviewed changes

cmsbuild added tests-started and removed tests-rejected labels Nov 1, 2021

cmsbuild added tests-approved and removed tests-started labels Nov 1, 2021

jpata reviewed Nov 2, 2021

View reviewed changes

small code improvements

2301a29

cmsbuild removed the tests-approved label Nov 4, 2021

code style

758843c

cmsbuild added code-checks-pending and removed code-checks-rejected labels Nov 9, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Nov 9, 2021

cmsbuild added tests-started and removed tests-pending labels Nov 9, 2021

cmsbuild added tests-rejected and removed tests-started labels Nov 9, 2021

cmsbuild added tests-started and removed tests-rejected labels Nov 9, 2021

cmsbuild added tests-approved and removed tests-started labels Nov 10, 2021

cmsbuild added fully-signed reconstruction-approved and removed reconstruction-pending pending-signatures labels Nov 11, 2021

cmsbuild added orp-approved and removed orp-pending labels Nov 11, 2021

cmsbuild merged commit d6e8724 into cms-sw:master Nov 11, 2021

cmsbuild mentioned this pull request Nov 11, 2021

Move to root 6.24 cms-sw/cmsdist#7445

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix charged and neutral candidate converters for reco::Jets #35922

Fix charged and neutral candidate converters for reco::Jets #35922

SWuchterl commented Oct 31, 2021

cmsbuild commented Oct 31, 2021

cmsbuild commented Oct 31, 2021

slava77 commented Oct 31, 2021

cmsbuild commented Oct 31, 2021

slava77 commented Oct 31, 2021

VinInn Nov 1, 2021

SWuchterl Nov 1, 2021 •

edited

VinInn Nov 1, 2021

jpata Nov 2, 2021

VinInn Nov 1, 2021

VinInn Nov 1, 2021

SWuchterl Nov 1, 2021

VinInn Nov 1, 2021

jpata Nov 2, 2021

AlexDeMoor Nov 2, 2021

slava77 commented Nov 1, 2021

cmsbuild commented Nov 1, 2021

jpata left a comment

jpata Nov 2, 2021

jpata Nov 2, 2021

cmsbuild commented Nov 9, 2021

cmsbuild commented Nov 9, 2021

jpata commented Nov 9, 2021

cmsbuild commented Nov 9, 2021

slava77 commented Nov 9, 2021

cmsbuild commented Nov 10, 2021

jpata commented Nov 11, 2021

cmsbuild commented Nov 11, 2021

qliphy commented Nov 11, 2021

Fix charged and neutral candidate converters for reco::Jets #35922

Fix charged and neutral candidate converters for reco::Jets #35922

Conversation

SWuchterl commented Oct 31, 2021

PR description:

PR validation:

if this PR is a backport please specify the original PR and why you need to backport that PR:

cmsbuild commented Oct 31, 2021

cmsbuild commented Oct 31, 2021

slava77 commented Oct 31, 2021

cmsbuild commented Oct 31, 2021

RelVals

RelVals-INPUT

AddOn Tests

slava77 commented Oct 31, 2021

Choose a reason for hiding this comment

SWuchterl Nov 1, 2021 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slava77 commented Nov 1, 2021

cmsbuild commented Nov 1, 2021

Comparison Summary

jpata left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmsbuild commented Nov 9, 2021

cmsbuild commented Nov 9, 2021

jpata commented Nov 9, 2021

cmsbuild commented Nov 9, 2021

RelVals

slava77 commented Nov 9, 2021

cmsbuild commented Nov 10, 2021

Comparison Summary

jpata commented Nov 11, 2021

cmsbuild commented Nov 11, 2021

qliphy commented Nov 11, 2021

SWuchterl Nov 1, 2021 •

edited