Phase2 L1T: Correlator development for AR2026#51270
Open
cerminar wants to merge 9 commits into
Open
Conversation
* Added nprong score to DataFormats. * Added L1TSC82ProngJet to DPGAnalysis. * Added L1TSC82ProngJetModel to L1Trigger/Phase2L1ParticleFlow * Fix for nprong tag. * Cleaned includes. * File renamed. * Removed comment from addPh2GTObjects(process) * fixes. * Reverted changes. * Removed cuts.. * Update L1Trigger/Phase2L1ParticleFlow/src/L1TSC82ProngJetID.cc Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update L1Trigger/Phase2L1ParticleFlow/plugins/L1TSC82ProngJetModelProducer.cc Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update DataFormats/L1TParticleFlow/src/jets.cpp Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * added values for minPt and maxEta. * Added std headers. * Added a check for fNParticles. * Update L1Trigger/Phase2L1ParticleFlow/python/l1pfJetMet_cff.py Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Apply CMSSW code-format --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Contributor
|
cms-bot internal usage |
Contributor
|
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-51270/49859
Code check has found code style and quality issues which could be resolved by applying following patch(s)
|
cerminar
force-pushed
the
20_1_X_CLT_dev_v0
branch
from
4f9ce87 to
2244a3a
Compare
Contributor
|
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-51270/49878
Code check has found code style and quality issues which could be resolved by applying following patch(s)
|
cerminar
force-pushed
the
20_1_X_CLT_dev_v0
branch
from
2244a3a to
20d60f1
Compare
Contributor
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-51270/49880
|
Contributor
|
A new Pull Request was created by @cerminar for master. It involves the following packages:
@BenjaminRS, @battibass, @cmsbuild, @ftorrresd, @quinnanm can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
cerminar
force-pushed
the
20_1_X_CLT_dev_v0
branch
from
20d60f1 to
9f52331
Compare
quinnanm
reviewed
Jun 22, 2026
Comment on lines
+78
to
+88
| <class name="l1tp2::DigitizedL1CaloJet" ClassVersion="5"> | ||
| <version ClassVersion="5" checksum="2456665692"/> | ||
| </class> | ||
| <class name="std::vector<l1tp2::DigitizedL1CaloJet>" /> | ||
| <class name="l1tp2::DigitizedL1CaloJetCollection" /> | ||
| <class name="edm::Wrapper<l1tp2::DigitizedL1CaloJetCollection>" /> | ||
|
|
||
| <class name="l1tp2::DigitizedCaloToCorrelatorTM18" ClassVersion="9"> | ||
| <version ClassVersion="9" checksum="3355885888"/> | ||
| </class> | ||
| <class name="l1tp2::DigitizedCaloToCorrelatorCollectionTM18" /> |
Contributor
There was a problem hiding this comment.
why does this start ClassVersion==5 and 9 seen here? previous class versions should be included if possible, see also DataFormats/L1TCorrelator/src/classes_def.xml, there may also be other examples
Contributor
Author
There was a problem hiding this comment.
Well some of the old versions are not relevant after adding the io_v1 namespace.
BTW, I am not sure why not all dataformats are declared in the enw namespace. @makortel ?
Contributor
There was a problem hiding this comment.
We moved only types stored in files in a phase 2 workflow into io_v1 namespace so that the 2026 phase 2 MC can be kept readable in the future while some data types will be evolved more drastically than in the past. Generally types for which backwards compatibility is important would be good to place in io_v1.
Note that I won't be responsive for the next ~3 weeks.
| ap_uint<6> fb() const { return ((clusterData >> 38) & 0x3F); } | ||
|
|
||
| // Encoding region information | ||
| ap_uint<36> spare() const { return ((clusterData >> 44) & 0xFFFFF); } |
Contributor
There was a problem hiding this comment.
should this be ap_uint<20> spare to match constructor?
| return (ap_uint<16>)(pt_f / LSB_PT); | ||
| } | ||
|
|
||
| ap_uint<13> digitizePhi(float phi_f) { |
Contributor
There was a problem hiding this comment.
should this be ap_int<13> ?
Contributor
Author
There was a problem hiding this comment.
This is a question for @pallabidas @asavincms. Also, it looks like the negative clamping might be off by 1.
| return (ap_uint<13>)(phi_f / LSB_PHI); | ||
| } | ||
|
|
||
| ap_uint<14> digitizeEta(float eta_f) { |
Contributor
There was a problem hiding this comment.
should this be ap_int<14>
Contributor
Author
There was a problem hiding this comment.
|
|
||
| class DigitizedCaloToCorrelatorTM18 { | ||
| private: | ||
| // Data (to remove) |
Contributor
There was a problem hiding this comment.
is this intentionally left in or should it be removed for the PR? could you add more detail to the comment if so?
|
|
||
| L1TCtL2EgProducer::~L1TCtL2EgProducer() {} | ||
|
|
||
| edm::ParameterSetDescription L1TCtL2EgProducer::getParameterSetDescription() { |
|
|
||
| taggedJets.push_back(edmJet); | ||
| } | ||
| std::sort(taggedJets.begin(), taggedJets.end(), [](l1t::PFJet a, l1t::PFJet b) { return (a.pt() > b.pt()); }); |
Contributor
There was a problem hiding this comment.
could the lambda parameters be made constant ie (const l1t::PFJet& a, const l1t::PFJet& b)?
| } | ||
| std::sort(taggedJets.begin(), taggedJets.end(), [](l1t::PFJet a, l1t::PFJet b) { return (a.pt() > b.pt()); }); | ||
|
|
||
| std::unique_ptr<l1t::PFJetCollection> taggedJetsCollection(new l1t::PFJetCollection); |
Contributor
There was a problem hiding this comment.
could this be auto taggedJetsCollection = std::make_unique<l1t::PFJetCollection>(); instead?
| edmJet.setEncodedJet(l1t::PFJet::HWEncoding::GTWide, gtwHWJet.pack()); | ||
|
|
||
| std::vector<edm::Ptr<l1t::PFCandidate>> constituents; | ||
| std::for_each(srcjet.constituents().begin(), srcjet.constituents().end(), [&](auto constituent) { |
Contributor
There was a problem hiding this comment.
why is this not a standard for loop, ie for (const auto& constituent : srcjet.constituents()) ?
| // 1 9 3 8 2 | ||
| // 2 11 5 10 4 | ||
|
|
||
| unsigned int L1TCorrelatorLayer1Producer::emDecodedIndex(unsigned int linkidx, unsigned int entidx) const { |
Contributor
There was a problem hiding this comment.
this file has a lot of magic number constants (81, 98, 138) - would it be better to store these as named constants in a shared header, especially if they are used in other files (I'm not sure if these are the same as used in Phase2L1CaloToCorrelatorTM18.cc for example)
Contributor
There was a problem hiding this comment.
yes these are same as in Phase2L1CaloToCorrelatorTM18.cc, will declare them in a common header
cmsbuild
added
tests-pending
code-checks-pending
and removed
tests-rejected
code-checks-approved
labels
Contributor
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-51270/49900
|
Contributor
|
Pull request #51270 was updated. @BenjaminRS, @battibass, @cmsbuild, @ftorrresd, @quinnanm can you please check and sign again. |
Contributor
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-51270/49902
|
Contributor
|
Pull request #51270 was updated. @BenjaminRS, @battibass, @cmsbuild, @ftorrresd, @quinnanm can you please check and sign again. |
cerminar
force-pushed
the
20_1_X_CLT_dev_v0
branch
from
a5b88bd to
f6fb003
Compare
Contributor
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-51270/49920
|
Contributor
|
Pull request #51270 was updated. @BenjaminRS, @battibass, @cmsbuild, @ftorrresd, @quinnanm can you please check and sign again. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
12 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR description:
Port recent Correlator development to master.
For now:
20X version of #51093
The PR needs data in cms-data/L1Trigger-Phase2L1ParticleFlow#11
PR validation:
https://cerminar.web.cern.ch/cerminar/plots/CTL1_valid_140Xv1N0/