LowPtElectrons: fix for UL FastSim mini v2 and nano v9 workflows #35181

bainbrid · 2021-09-07T16:00:51Z

PR description:

UL mini v2 workflows for FastSim are broken due to an exception thrown by the LowPtGsfElectronIDProducer when attempting to access an edm::Ref to the missing generalTracksBeforeMixing collection in AOD.
All details can be found in this issue: Mini-v2 can not be produced in FastSim UL scenario #34774.
This PR provides an acceptable fix in the RECO chain by suppressing (i.e. removing) the FastSim switch to using tracks from the generalTracksBeforeMixing collection in the GsfElectronCore and GsfElectron objects.
For re-miniaod (and re-nanoaod) workflows, this PR also provides an acceptable workaround that allows the ID producer to evaluate the BDT model without throwing an exception, while maintaining the expected physics performance. This is achieved by extracting features from the KF track used in the ElectronSeed step (obtained from the available generalTracks collection in AOD), instead of from the KF track embedded in the GsfElectronCore object (which is chosen based on number of shared hits with the GsfTrack and obtained from the missing generalTracksBeforeMixing collection in AOD).
All development was performed in the 10_6_X cycle (see the compare here), tested with the example recipes provided in the issue #34774, and then ported to master (this PR). This work will be back ported to 10_6_X in a future PR.
Also tested is the behaviour for the nano v9 workflow, which is similarly affected.
This work was presented to RECO/AT here: https://indico.cern.ch/event/1071803/. The final slide compares physics performance b/w the 10_6_X and 12_1_X cycles (essentially identical).
Physics and computing performances are ~unaffected.

PR validation:

This PR was validated using cmsDriver commands that performed the GEN->AOD and mini v2 steps, based on those provided here, for tests with the 10_6_X cycle, and modified versions for tests with the 12_1_X release (this PR).

@slava77 @jordan-martins @crovelli

…2 and nano v9, issue cms-sw#34774

cmsbuild · 2021-09-07T16:07:36Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35181/25114

This PR adds an extra 20KB to repository

cmsbuild · 2021-09-07T16:07:59Z

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

It involves the following packages:

RecoEgamma/EgammaElectronProducers (reconstruction)

@jpata, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@Sam-Harper, @jainshilpi, @rovere, @lgray, @sobhatta, @lecriste, @afiqaize, @varuns23, @ram1123 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-09-08T22:38:12Z

@cmsbuild please test

cmsbuild · 2021-09-09T06:36:31Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-b28116/18429/summary.html
COMMIT: 11131ac
CMSSW: CMSSW_12_1_X_2021-09-08-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/35181/18429/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: 2 differences found in the comparisons
DQMHistoTests: Total files compared: 39
DQMHistoTests: Total histograms compared: 3001001
DQMHistoTests: Total failures: 0
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3000979
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 38 files compared)
Checked 165 log files, 37 edm output root files, 39 DQM output files
TriggerResults: no differences found

slava77 · 2021-09-09T17:58:09Z

RecoEgamma/EgammaElectronProducers/plugins/LowPtGsfElectronIDProducer.cc

+            const edm::Ptr<pat::PackedCandidate>* ptr1 =
+                ele.hasUserData("ele2packed") ? ele.template userData<edm::Ptr<pat::PackedCandidate>>("ele2packed")
+                                              : nullptr;
+            const edm::Ptr<pat::PackedCandidate>* ptr2 =
+                ele.hasUserData("ele2lost") ? ele.template userData<edm::Ptr<pat::PackedCandidate>>("ele2lost")
+                                            : nullptr;


Suggested change

const edm::Ptr<pat::PackedCandidate>* ptr1 =

ele.hasUserData("ele2packed") ? ele.template userData<edm::Ptr<pat::PackedCandidate>>("ele2packed")

: nullptr;

const edm::Ptr<pat::PackedCandidate>* ptr2 =

ele.hasUserData("ele2lost") ? ele.template userData<edm::Ptr<pat::PackedCandidate>>("ele2lost")

: nullptr;

using PackedPtr = edm::Ptr<pat::PackedCandidate>;

const PackedPtr* ptr1 = ele.template userData<PackedPtr>("ele2packed");

const PackedPtr* ptr1 = ele.template userData<PackedPtr>("ele2lost");

less verbose

PATObject::userData already returns nullptr if data is not available

slava77 · 2021-09-09T18:04:16Z

RecoEgamma/EgammaElectronProducers/plugins/LowPtGsfElectronIDProducer.cc

+            if (ptr1 != nullptr && ptr1->isNonnull() && ptr1->isAvailable() && ptr1->get() != nullptr &&
+                ptr1->get()->bestTrack() != nullptr) {
+              return ptr1->get()->bestTrack();
+            } else if (ptr2 != nullptr && ptr2->isNonnull() && ptr2->isAvailable() && ptr2->get() != nullptr &&
+                       ptr2->get()->bestTrack() != nullptr) {
+              return ptr2->get()->bestTrack();


the string of checks is a bit elaborate and repeated

Suggested change

if (ptr1 != nullptr && ptr1->isNonnull() && ptr1->isAvailable() && ptr1->get() != nullptr &&

ptr1->get()->bestTrack() != nullptr) {

return ptr1->get()->bestTrack();

} else if (ptr2 != nullptr && ptr2->isNonnull() && ptr2->isAvailable() && ptr2->get() != nullptr &&

ptr2->get()->bestTrack() != nullptr) {

return ptr2->get()->bestTrack();

bool hasBestTrack = [] (const PackedPtr* ptr) {

return ptr != nullptr && ptr->isNonnull() && ptr->isAvailable() && ptr->get() != nullptr &&

ptr->get()->bestTrack() != nullptr

};

if (hasBestTrack(ptr1)) {

return ptr1->get()->bestTrack();

} else if (hasBestTrack(ptr2)) {

return ptr2->get()->bestTrack();

plugins/LowPtGsfElectronIDProducer.cc:166:34: error: 'hasBestTrack' cannot be used as a function 166 | if (hasBestTrack(ptr1)) { | ^ ... plugins/LowPtGsfElectronIDProducer.cc:162:18: error: the address of 'static constexpr bool LowPtGsfElectronIDProducer::produce(edm::StreamID, edm::Event&, const edm::EventSetup&) const::<lambda(const auto:23&)> [with auto:23 = pat::Electron]::<lambda(const PackedPtr*)>::_FUN(const PackedPtr*)' will never be NULL [-Werror=address]

@slava77 ?

please try auto hasBestTrack = and ; at the end of return.

slava77 · 2021-09-09T18:21:10Z

RecoEgamma/EgammaElectronProducers/src/LowPtGsfElectronFeatures.cc

+    if (trk != nullptr) {
+      eid_trk_p = (float)trk->p();
+      eid_trk_nhits = (float)trk->found();
+      eid_trk_chi2red = (float)trk->normalizedChi2();
+      TVector3 trkTV3(0, 0, 0);
+      trkTV3.SetPtEtaPhi(trk->pt(), trk->eta(), trk->phi());
+      TVector3 eleTV3(0, 0, 0);
+      eleTV3.SetPtEtaPhi(ele.pt(), ele.eta(), ele.phi());
+      trk_dr = eleTV3.DeltaR(trkTV3);
+    } else {
+      if (ele.core().isNonnull()) {
+        reco::TrackRef trk = ele.closestCtfTrackRef();
+        if (trk.isNonnull()) {
+          eid_trk_p = (float)trk->p();
+          eid_trk_nhits = (float)trk->found();
+          eid_trk_chi2red = (float)trk->normalizedChi2();
+          TVector3 trkTV3(0, 0, 0);
+          trkTV3.SetPtEtaPhi(trk->pt(), trk->eta(), trk->phi());
+          TVector3 eleTV3(0, 0, 0);
+          eleTV3.SetPtEtaPhi(ele.pt(), ele.eta(), ele.phi());
+          trk_dr = eleTV3.DeltaR(trkTV3);
+        }


Suggested change

if (trk != nullptr) {

eid_trk_p = (float)trk->p();

eid_trk_nhits = (float)trk->found();

eid_trk_chi2red = (float)trk->normalizedChi2();

TVector3 trkTV3(0, 0, 0);

trkTV3.SetPtEtaPhi(trk->pt(), trk->eta(), trk->phi());

TVector3 eleTV3(0, 0, 0);

eleTV3.SetPtEtaPhi(ele.pt(), ele.eta(), ele.phi());

trk_dr = eleTV3.DeltaR(trkTV3);

} else {

if (ele.core().isNonnull()) {

reco::TrackRef trk = ele.closestCtfTrackRef();

if (trk.isNonnull()) {

eid_trk_p = (float)trk->p();

eid_trk_nhits = (float)trk->found();

eid_trk_chi2red = (float)trk->normalizedChi2();

TVector3 trkTV3(0, 0, 0);

trkTV3.SetPtEtaPhi(trk->pt(), trk->eta(), trk->phi());

TVector3 eleTV3(0, 0, 0);

eleTV3.SetPtEtaPhi(ele.pt(), ele.eta(), ele.phi());

trk_dr = eleTV3.DeltaR(trkTV3);

}

if (trk != nullptr || (ele.core().isNonnull() && ele.closestCtfTrackRef().isNonnull()) {

const reco::Track* tk = trk ? trk : &*ele.closestCtfTrackRef().isNonnull();

eid_trk_p = tk->p();

eid_trk_nhits = tk->found();

eid_trk_chi2red = tk->normalizedChi2();

TVector3 trkTV3(0, 0, 0);

trkTV3.SetPtEtaPhi(tk->pt(), tk->eta(), tk->phi());

TVector3 eleTV3(0, 0, 0);

eleTV3.SetPtEtaPhi(ele.pt(), ele.eta(), ele.phi());

trk_dr = reco::deltaR(*tk, ele);

}

remove repeated code

remove local trk shadowing the input trk

drop (float), which is already clear from lhs

remove unnecessary and pretty heavy conversion to TVector3

cmsbuild · 2021-09-10T17:31:08Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35181/25188

This PR adds an extra 20KB to repository

cmsbuild · 2021-09-10T17:31:29Z

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

slava77 · 2021-09-10T17:36:52Z

@cmsbuild please test

RecoEgamma/EgammaElectronProducers/src/LowPtGsfElectronFeatures.cc

Co-authored-by: Slava Krutelyov <slava77@gmail.com>

cmsbuild · 2021-09-14T18:09:56Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35181/25266

This PR adds an extra 24KB to repository

cmsbuild · 2021-09-14T18:10:20Z

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

slava77 · 2021-09-14T18:16:06Z

@cmsbuild please test

cmsbuild · 2021-09-14T21:29:18Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-b28116/18606/summary.html
COMMIT: 0212d1d
CMSSW: CMSSW_12_1_X_2021-09-14-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/35181/18606/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: 8 differences found in the comparisons
DQMHistoTests: Total files compared: 39
DQMHistoTests: Total histograms compared: 3000833
DQMHistoTests: Total failures: 12
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3000799
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 38 files compared)
Checked 165 log files, 37 edm output root files, 39 DQM output files
TriggerResults: no differences found

slava77 · 2021-09-15T15:38:45Z

+reconstruction

for #35181 0212d1d

code changes are in line with the PR description and the follow up review and the preceding discussion in Mini-v2 can not be produced in FastSim UL scenario #34774
jenkins tests pass and comparisons with the baseline show differences in lowPtGsfElectrons.convVtxFitProb which is computed using ctfTrack, which is modified by this PR
I tested locally that an AODSIM fastsim file produced in CMSSW_10_6_22 can be processed now with --step PAT --procModifiers run2_miniAOD_UL --fast (in line with the PR description)

cmsbuild · 2021-09-15T15:39: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. @perrotta, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

slava77 · 2021-09-16T22:59:38Z

@bainbrid
it may be practical to prepare a PR already for 10_6_X.

qliphy · 2021-09-17T01:32:56Z

+1

bainbrid · 2021-09-17T08:16:49Z

@bainbrid
it may be practical to prepare a PR already for 10_6_X.

Yes, I already have the branch ready, just need to fold in your suggested changes. Will try to make the PR today.

bainbrid added 2 commits September 7, 2021 17:25

do NOT take 'closest' track from generalTracksBeforeMixing for FastSim

f661be2

ID producer uses seed track instead of closest, for UL FastSim mini v…

11131ac

…2 and nano v9, issue cms-sw#34774

cmsbuild added this to the CMSSW_12_1_X milestone Sep 7, 2021

cmsbuild added code-checks-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Sep 7, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Sep 7, 2021

cmsbuild added tests-started and removed tests-pending labels Sep 8, 2021

cmsbuild added tests-approved and removed tests-started labels Sep 9, 2021

slava77 reviewed Sep 9, 2021

View reviewed changes

slava review

dd17b74

cmsbuild added code-checks-pending tests-pending and removed tests-approved code-checks-approved labels Sep 10, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Sep 10, 2021

cmsbuild removed the tests-pending label Sep 10, 2021

slava77 reviewed Sep 14, 2021

View reviewed changes

RecoEgamma/EgammaElectronProducers/src/LowPtGsfElectronFeatures.cc Outdated Show resolved Hide resolved

Slava review

0212d1d

Co-authored-by: Slava Krutelyov <slava77@gmail.com>

cmsbuild added code-checks-pending tests-pending and removed tests-approved code-checks-approved labels Sep 14, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Sep 14, 2021

cmsbuild added tests-started and removed tests-pending labels Sep 14, 2021

cmsbuild added tests-approved and removed tests-started labels Sep 14, 2021

cmsbuild added fully-signed reconstruction-approved and removed reconstruction-pending pending-signatures labels Sep 15, 2021

cmsbuild added orp-approved and removed orp-pending labels Sep 17, 2021

cmsbuild merged commit 43eb090 into cms-sw:master Sep 17, 2021

cmsbuild mentioned this pull request Sep 20, 2021

Update Geant4 to 10.7.2 cms-sw/cmsdist#7316

Merged

bainbrid mentioned this pull request Sep 20, 2021

LowPtElectrons: fix for UL FastSim mini v2 and nano v9 workflows (back port) #35341

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LowPtElectrons: fix for UL FastSim mini v2 and nano v9 workflows #35181

LowPtElectrons: fix for UL FastSim mini v2 and nano v9 workflows #35181

bainbrid commented Sep 7, 2021

cmsbuild commented Sep 7, 2021

cmsbuild commented Sep 7, 2021

slava77 commented Sep 8, 2021

cmsbuild commented Sep 9, 2021

slava77 Sep 9, 2021

bainbrid Sep 10, 2021

slava77 Sep 9, 2021

bainbrid Sep 10, 2021 •

edited

slava77 Sep 10, 2021

bainbrid Sep 10, 2021

slava77 Sep 9, 2021

bainbrid Sep 10, 2021

cmsbuild commented Sep 10, 2021

cmsbuild commented Sep 10, 2021

slava77 commented Sep 10, 2021

cmsbuild commented Sep 14, 2021

cmsbuild commented Sep 14, 2021

slava77 commented Sep 14, 2021

cmsbuild commented Sep 14, 2021

slava77 commented Sep 15, 2021

cmsbuild commented Sep 15, 2021

slava77 commented Sep 16, 2021

qliphy commented Sep 17, 2021

bainbrid commented Sep 17, 2021

LowPtElectrons: fix for UL FastSim mini v2 and nano v9 workflows #35181

LowPtElectrons: fix for UL FastSim mini v2 and nano v9 workflows #35181

Conversation

bainbrid commented Sep 7, 2021

PR description:

PR validation:

cmsbuild commented Sep 7, 2021

cmsbuild commented Sep 7, 2021

slava77 commented Sep 8, 2021

cmsbuild commented Sep 9, 2021

Comparison Summary

slava77 Sep 9, 2021

Choose a reason for hiding this comment

bainbrid Sep 10, 2021

Choose a reason for hiding this comment

slava77 Sep 9, 2021

Choose a reason for hiding this comment

bainbrid Sep 10, 2021 • edited

Choose a reason for hiding this comment

slava77 Sep 10, 2021

Choose a reason for hiding this comment

bainbrid Sep 10, 2021

Choose a reason for hiding this comment

slava77 Sep 9, 2021

Choose a reason for hiding this comment

bainbrid Sep 10, 2021

Choose a reason for hiding this comment

cmsbuild commented Sep 10, 2021

cmsbuild commented Sep 10, 2021

slava77 commented Sep 10, 2021

cmsbuild commented Sep 14, 2021

cmsbuild commented Sep 14, 2021

slava77 commented Sep 14, 2021

cmsbuild commented Sep 14, 2021

Comparison Summary

slava77 commented Sep 15, 2021

cmsbuild commented Sep 15, 2021

slava77 commented Sep 16, 2021

qliphy commented Sep 17, 2021

bainbrid commented Sep 17, 2021

bainbrid Sep 10, 2021 •

edited