Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consolidation of Phase-2 e/g object interface #1181

Conversation

cerminar
Copy link

@cerminar cerminar commented Nov 17, 2023

PR description:

This PR addresses several issues with the interface of TkElectron and TkEm objects and the HW objects used by the Phase2 GT emulator:

  • add ID score to both the hardware and CMSSW electron objects;
  • add interface to retrieve object in GT hw format;
  • remove Ref. to Egamma standalone objects since they are not the actual ancestors due to architectural choice in L1T hardware. The ref is replaced by an edm::Ptr to the calorimeter primitive (different in barrel and endcap).
  • further cleanup of the TkElectron interface and of data members;
  • charge is now populated for all TkElectrons;
  • change charge definition for GT electrons in accordance with new convention;
  • use non-PV-based photon isolation in hardware objects;
  • eta and phi at vertex (from the track) are now used for the TkElectron coordinates.

NOTE: backward compatibility is maintained via IO rule in the reflex dictionary.

PR validation:

Successfully tested reading files produced with previous versions of the object data formats.

@epalencia epalencia added the Phase-2 Pertains to phase-2 development label Nov 17, 2023
Copy link

@gpetruc gpetruc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable to me

refRemapper.origRefAndPtr.push_back(std::make_pair(refEg, tkele.trkPtr()));
emu.sta_idx = refRemapper.origRefAndPtr.size() - 1;

// FIXME: this is hugly
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

@@ -87,15 +87,16 @@ namespace l1ct {
z0_t hwZ0;
tkdeta_t hwDEta; // relative to the region center, at calo
tkdphi_t hwDPhi; // relative to the region center, at calo
id_score_t hwIDScore;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW the two comments above on hwDEta and hwDPhi are misleading

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

Copy link

@gpetruc gpetruc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But perhaps better to use the standard defaultconstructor for null Ptrs rather than the constructor for a temporary ptr object with a null ptr

if (trkPtr_.isNonnull()) {
setTrkzVtx(trkPtr()->POCA().z());
}
}

TkElectron::TkElectron(const LorentzVector& p4, float tkisol)
: TkElectron(p4, edm::Ptr<L1Candidate>(nullptr, 0), edm::Ptr<L1TTTrackType>(nullptr, 0), tkisol) {}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
: TkElectron(p4, edm::Ptr<L1Candidate>(nullptr, 0), edm::Ptr<L1TTTrackType>(nullptr, 0), tkisol) {}
: TkElectron(p4, edm::Ptr<L1Candidate>(), edm::Ptr<L1TTTrackType>(), tkisol) {}

TkEm::TkEm(const LorentzVector& p4, const edm::Ref<EGammaBxCollection>& egRef, float tkisol)
: TkEm(p4, egRef, tkisol, -999) {}
TkEm::TkEm(const LorentzVector& p4, float tkisol, float tkisolPV)
: TkEm(p4, edm::Ptr<L1Candidate>(nullptr, 0), tkisol, tkisolPV) {}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
: TkEm(p4, edm::Ptr<L1Candidate>(nullptr, 0), tkisol, tkisolPV) {}
: TkEm(p4, edm::Ptr<L1Candidate>(), tkisol, tkisolPV) {}

}
refRemapper.origRefAndPtr.push_back(std::make_pair(refEg, edm::Ptr<RefRemapper::L1TTTrackType>(nullptr, 0)));
emu.sta_idx = refRemapper.origRefAndPtr.size() - 1;
constituentsPtrs.push_back(std::make_pair(tkem.egCaloPtr(), edm::Ptr<L1TTTrackType>(nullptr, 0)));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
constituentsPtrs.push_back(std::make_pair(tkem.egCaloPtr(), edm::Ptr<L1TTTrackType>(nullptr, 0)));
constituentsPtrs.push_back(std::make_pair(tkem.egCaloPtr(), edm::Ptr<L1TTTrackType>()));

@cerminar
Copy link
Author

PR to master is cms-sw#43317

@cerminar cerminar marked this pull request as ready for review November 17, 2023 16:14
@aloeliger aloeliger added the Data types Anything related to development of datatypes for L1 use label Nov 17, 2023
@triggerDoctor
Copy link

Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation.

Attempts to compile this PR succeeded!

Info Value
return code 0
command eval scramv1 runtime -sh && scram b -j 8

@triggerDoctor
Copy link

Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation.

I found no issues with the code checks!

Info Value
return code 0
command eval scramv1 runtime -sh && scram b -k -j 8 code-checks && scram b -k -j 8 code-checks

I found no issues with the headers!

Info Value
return code 0
command eval scramv1 runtime -sh && scram b -k -j 8 check-headers

@triggerDoctor
Copy link

Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation.

I found no files with code format issues!

Info Value
return code 0
command eval scramv1 runtime -sh && scram b -k -j 8 code-format-all

@epalencia epalencia merged commit 3ad78a5 into cms-l1t-offline:phase2-l1t-integration-13_3_0_pre3 Nov 22, 2023
@epalencia
Copy link

Tagged as phase2-l1t-1330pre3_v6.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Data types Anything related to development of datatypes for L1 use Phase-2 Pertains to phase-2 development
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants