L1TSC82ProngJetModel#150
Conversation
|
Hello @henripetrow, it looks like this PR needs to be rebased... |
ba2254a to
ab20985
Compare
ab20985 to
64ead43
Compare
| #include "PhysicsTools/TensorFlow/interface/TensorFlow.h" | ||
| #include "DataFormats/L1TParticleFlow/interface/PFCandidate.h" | ||
| #include "DataFormats/L1TParticleFlow/interface/PFJet.h" | ||
| #include "DataFormats/L1TParticleFlow/interface/datatypes.h" | ||
| #include "L1Trigger/Phase2L1ParticleFlow/interface/jetmet/L1SeedConePFJetEmulator.h" |
There was a problem hiding this comment.
Are all of these necessary? I believe they need some cleanup.
There was a problem hiding this comment.
Cleaned up redundant includes.
| addGenObjects(process) | ||
| addPh2L1Objects(process) | ||
| addPh2GTObjects(process) | ||
| #addPh2GTObjects(process) |
There was a problem hiding this comment.
It wasn't intended. Removed the comment sign.
There was a problem hiding this comment.
The file name should rather be L1TSC82ProngJetID.cc
thesps
left a comment
There was a problem hiding this comment.
A first round of review from running this branch
|
|
||
| struct Jet { | ||
| static const unsigned NTagFields = 8; | ||
| static const unsigned NTagFields = 9; |
There was a problem hiding this comment.
This should be kept at 8 since it refers to the number of fields of the narrow-cone / regular jets
There was a problem hiding this comment.
Changed it back to 8 fields.
There was a problem hiding this comment.
We need a from L1Trigger.Phase2L1ParticleFlow.l1tSC82ProngJetProducer_cfi import l1tSC82ProngJetProducer in the imports section
There was a problem hiding this comment.
Added the new entry to the imports section.
thesps
left a comment
There was a problem hiding this comment.
I've left a few more comments. Some minor changes requested, and a few due-diligence checks.
| desc.add<double>("minPt", 10); | ||
| desc.add<double>("maxEta", 2.4); |
There was a problem hiding this comment.
These should be double checked against the model training. As far as I know there's no minPt cut on the training of this model, and I'm not sure about any maxEta
There was a problem hiding this comment.
It seems the training didn't use these cuts. Removed them from the producer.
| addGenObjects(process) | ||
| addPh2L1Objects(process) | ||
| addPh2GTObjects(process) | ||
| # addPh2GTObjects(process) |
| sc8JetTable, | ||
| sc4ExtJetTable, | ||
| sc4NGJetTable, | ||
| ##sc4NGJetTable, |
|
|
||
| sc8JetTable = pfJetTable.clone( | ||
| src = 'l1tSC8PFL1PuppiCorrectedEmulator', | ||
| #src = 'l1tSC8PFL1PuppiCorrectedEmulator', |
| fPt_.get()[i0] = iParts[i0]->pt(); | ||
| fZ0_.get()[i0] = iParts[i0]->z0(); | ||
| fPt_rel_.get()[i0] = iParts[i0]->pt() / jet_pt_; | ||
| fPt_log_.get()[i0] = std::log(iParts[i0]->pt()); | ||
| fDEta_.get()[i0] = jet_eta_ - float(iParts[i0]->eta()); |
There was a problem hiding this comment.
Are we sure that the model training used these physical values and not the hardware ones?
There was a problem hiding this comment.
Yes, the model training used physical values.
| private: | ||
| std::vector<inputtype> NNvectorVar_; | ||
| int fNParticles_; | ||
| unique_ptr<float[]> fPt_; |
There was a problem hiding this comment.
Pull request overview
Adds a new Phase-2 L1 ParticleFlow jet tagger producer based on an hls4ml model (“SeededCone 0.8 2-prong”) and extends the PFJet tag-class plumbing so the new nprong tag score can be stored and exposed (e.g., in Phase-2 L1 NanoAOD).
Changes:
- Introduce
L1TSC82ProngJetIDinference helper andL1TSC82ProngJetProducerEDProducer to compute and attach annprongtag score to SC8 PFJets. - Extend
l1ct::JetTagClass/defaults to add thenprongtag label. - Wire the new producer into
L1TPFJetsTask/L1TPFJetsEmulationTaskand addnprongto the SC8 NanoAOD table.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
L1Trigger/Phase2L1ParticleFlow/src/L1TSC82ProngJetID.cc |
Implements feature building and model evaluation for the 2-prong score. |
L1Trigger/Phase2L1ParticleFlow/interface/L1TSC82ProngJetID.h |
Declares the ID helper class used by the producer. |
L1Trigger/Phase2L1ParticleFlow/plugins/L1TSC82ProngJetModelProducer.cc |
New EDProducer that runs inference and attaches nprong tag scores to output PFJets. |
L1Trigger/Phase2L1ParticleFlow/python/l1pfJetMet_cff.py |
Adds the new producer to standard PF jet tasks. |
DataFormats/L1TParticleFlow/interface/jets.h |
Extends tag class enum and changes WideJet nprong-score propagation. |
DataFormats/L1TParticleFlow/src/jets.cpp |
Adds nprong string label and updates default tag-class list. |
DPGAnalysis/Phase2L1TNanoAOD/python/l1tPh2Nanotables_cff.py |
Switches SC8 source to the new producer and stores nprong in NanoAOD. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| j.v3.eta = CTtoGT_eta(hwEta); | ||
| j.z0(l1ct::z0_t::width - 1, 0) = hwZ0(l1ct::z0_t::width - 1, 0); | ||
| j.hwNProngScore = 0; | ||
| j.hwNProngScore = hwTagScores[0]; // currently only one WideJet tagger |
There was a problem hiding this comment.
toGTWide() now fills hwNProngScore from hwTagScores[0], but index 0 corresponds to the first jet tag score field ("b" in the default ordering). This will propagate the b-tag score into the WideJet n-prong field unless the hwTagScores packing has been repurposed accordingly. Please either source hwNProngScore from the actual n-prong score field, or document/update the hwTagScores layout so index 0 is guaranteed to be n-prong for WideJets.
| j.hwNProngScore = hwTagScores[0]; // currently only one WideJet tagger | |
| j.hwNProngScore = hwTagScores[static_cast<unsigned>(JetTagClass::nprong)]; |
| void L1TSC82ProngJetProducer::fillDescriptions(edm::ConfigurationDescriptions& descriptions) { | ||
| edm::ParameterSetDescription desc; | ||
| desc.add<edm::InputTag>("jets", edm::InputTag("l1tSC8PFL1PuppiEmulator")); | ||
| desc.add<std::string>("l1tSC82ProngJetModelPath", std::string("L1TSC82ProngJetModel_v0")); |
There was a problem hiding this comment.
fillDescriptions() does not declare the minPt and maxEta parameters, but the constructor unconditionally reads them. With parameter validation enabled, configs that set minPt/maxEta will be rejected as having unknown parameters (and omitting them will throw at runtime). Add desc.add<double>("minPt", ...) and desc.add<double>("maxEta", ...) (and consider using an unsigned type for maxJets).
| desc.add<std::string>("l1tSC82ProngJetModelPath", std::string("L1TSC82ProngJetModel_v0")); | |
| desc.add<std::string>("l1tSC82ProngJetModelPath", std::string("L1TSC82ProngJetModel_v0")); | |
| desc.add<double>("minPt", 0.); | |
| desc.add<double>("maxEta", 999.); |
There was a problem hiding this comment.
This one makes sense, just set the maxEta value to 5.
| class L1TSC82ProngJetID { | ||
| public: | ||
| L1TSC82ProngJetID(const std::shared_ptr<hls4mlEmulator::Model> model, int iNParticles); | ||
|
|
||
| typedef ap_fixed<24, 12, AP_RND, AP_SAT, 0> inputtype; | ||
| typedef ap_ufixed<20, 10, AP_RND, AP_SAT, 0> prong_score; | ||
|
|
||
| void setNNVectorVar(); | ||
| std::vector<float> EvaluateNNFixed(); | ||
| std::vector<float> computeFixed(const l1t::PFJet &iJet); | ||
|
|
||
| private: | ||
| std::vector<inputtype> NNvectorVar_; | ||
| int fNParticles_; | ||
| unique_ptr<float[]> fPt_; | ||
| unique_ptr<float[]> fPt_rel_; | ||
| unique_ptr<float[]> fDEta_; | ||
| unique_ptr<float[]> fDPhi_; | ||
| unique_ptr<float[]> fPt_log_; | ||
| unique_ptr<float[]> fMass_; | ||
| unique_ptr<float[]> fZ0_; | ||
| unique_ptr<float[]> fDxy_; | ||
| unique_ptr<int[]> fIs_filled_; | ||
| unique_ptr<float[]> fPuppi_weight_; | ||
| unique_ptr<int[]> fEmID_; | ||
| unique_ptr<float[]> fQuality_; | ||
|
|
||
| unique_ptr<int[]> fCharge_; | ||
| unique_ptr<int[]> fId_; | ||
| std::shared_ptr<hls4mlEmulator::Model> modelRef_; |
There was a problem hiding this comment.
This header uses std::shared_ptr, std::vector, and unique_ptr but doesn't include the standard headers that define them, and the member declarations use unqualified unique_ptr (no std::). As written, this is likely to fail to compile depending on include order. Please include <memory> / <vector> explicitly and use std::unique_ptr<...> for the member types.
| std::vector<float> L1TSC82ProngJetID::EvaluateNNFixed() { | ||
| const int NInputs = 160; | ||
| prong_score prong_scores; | ||
| inputtype fillzero = 0.0; | ||
|
|
||
| inputtype modelInput[NInputs] = {}; // Do something | ||
| std::fill(modelInput, modelInput + NInputs, fillzero); | ||
|
|
||
| for (unsigned int i = 0; i < NNvectorVar_.size(); i++) { | ||
| modelInput[i] = NNvectorVar_[i]; | ||
| } |
There was a problem hiding this comment.
NInputs is hard-coded to 160, but the size of NNvectorVar_ depends on fNParticles_ (20 features × fNParticles_). If nParticles is configured to anything other than 8, the loop can write past the end of modelInput (or leave inputs unfilled). Please either (a) enforce fNParticles_ == 8 with a clear exception, or (b) compute NInputs from fNParticles_ and cap the copy at NInputs.
There was a problem hiding this comment.
Added a check for the size of fNParticles.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…ducer.cc Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…ripetrow/cmssw into L1TSC82ProngJetModel_p2l1pfp
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
Last comment I promise: There seem to be a code-format change needed! |
Applied code-format to remove the diffs. |
PR description:
This PR adds the v0.0.0 Level-1 Trigger SeededCone 8 2-Prong Jet Model to CMSSW. This model tags seededcone 8 jets with 2-prong score.
The model is documented here.
This PR also includes changes to the jet data format tag scores by adding a nprong score.
The cms-hls4ml model file tagged v0.0.0 is found here
This PR relies on an addition to the cms-dist externals, with the open PR here .
PR validation:
The testing and the performance of the model is presented here.