-
Notifications
You must be signed in to change notification settings - Fork 4.6k
[L1T] Update L1T MET and Add JUMP algorithm (Phase 2) #48308
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
Conversation
|
cms-bot internal usage |
|
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-48308/45177 Code check has found code style and quality issues which could be resolved by applying following patch(s)
|
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-48308/45179 |
|
A new Pull Request was created by @JunwonTomOh for master. It involves the following packages:
@BenjaminRS, @Moanwar, @cmsbuild, @quinnanm, @srimanob, @subirsarkar can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
test parameters:
|
|
please test |
|
@JunwonTomOh can you update the title of this PR please, following the L1T recommendations, to: |
|
@BenjaminRS Thank you for the comments. I've just changed. |
|
+1 Size: This PR adds an extra 16KB to repository Comparison SummarySummary:
|
|
|
||
| L1METEmu::eta_t eta_edges[4]; | ||
| float eta_boundaries[4] = {1.3, 1.7, 2.5, 3.0}; | ||
| for (uint i = 0; i < 4; i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should uint be unsigned int? also other places in the code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, we can use the int type, but when we compare with .size(), it seems like we should use the uint type. For example, at line 76 in L1Trigger/Phase2L1ParticleFlow/interface/jetmet/L1PFJUMPEmulator.h, uint is used like this.
for (uint i = 0; i < jets.size(); i++) {
Get_dPt(jets[i], each_dPx2, each_dPy2);
sum_dPx2 += each_dPx2;
sum_dPy2 += each_dPy2;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll change the uint into int type if possible. Thanks for the comment and please feel free to share any advise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok if you think it is more appropriate feel free to use uint. this was more of a question than a strict suggestion!
| : inMet_xy.hwPy - L1METEmu::proj2_t(sqrt(dPy_2.to_float())); | ||
| L1METEmu::pxpy_to_ptphi(outMet_xy, outMet); | ||
|
|
||
| return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not a big deal, but dont need return; for a void function
| poly_t cos2_par0[16] = {-1.00007, | ||
| -0.924181, | ||
| -0.707596, | ||
| -0.382902, | ||
| -0.000618262, | ||
| 0.382137, | ||
| 0.707056, | ||
| 0.923708, | ||
| 1.00007, | ||
| 0.924181, | ||
| 0.707594, | ||
| 0.383285, | ||
| 0.000188727, | ||
| -0.382139, | ||
| -0.706719, | ||
| -0.923708}; | ||
| poly_t cos2_par1[16] = {9.164680268990924e-06, | ||
| 0.0017064607695524156, | ||
| 0.0031441321076514446, | ||
| 0.004079929656016374, | ||
| 0.004437063290882583, | ||
| 0.004095969231842202, | ||
| 0.0031107221424451436, | ||
| 0.001689531075808071, | ||
| -9.161756842493832e-06, | ||
| -0.001706456406229286, | ||
| -0.003143961938049376, | ||
| -0.004103015998697129, | ||
| -0.004411145151490469, | ||
| -0.0040958165155326525, | ||
| -0.0031310072316764474, | ||
| -0.001689531075808071}; | ||
| poly_t cos2_par2[16] = {9.319674765430664e-06, | ||
| 7.871694899063284e-06, | ||
| 5.222989318251642e-06, | ||
| 2.0256106486379287e-06, | ||
| -1.9299417402361656e-06, | ||
| -5.35167113952279e-06, | ||
| -7.740062096537953e-06, | ||
| -9.348822844786505e-06, | ||
| -9.319674765430664e-06, | ||
| -7.871694899063284e-06, | ||
| -5.225331064666252e-06, | ||
| -1.780776301343235e-06, | ||
| 1.6556927733433181e-06, | ||
| 5.3495197789955455e-06, | ||
| 7.954684107366423e-06, | ||
| 9.348822844786505e-06}; | ||
|
|
||
| poly_t sin2_par0[16] = {0.000524872, | ||
| -0.382229, | ||
| -0.706791, | ||
| -0.923959, | ||
| -1.00008, | ||
| -0.924156, | ||
| -0.707264, | ||
| -0.383199, | ||
| -0.000525527, | ||
| 0.382228, | ||
| 0.706792, | ||
| 0.923752, | ||
| 1.00013, | ||
| 0.924155, | ||
| 0.707535, | ||
| 0.3832}; | ||
| poly_t sin2_par1[16] = {-0.004431478237276202, | ||
| -0.00409041472149773, | ||
| -0.0031267268116859314, | ||
| -0.00167440343451641, | ||
| 9.741773386162849e-06, | ||
| 0.0017049641497188307, | ||
| 0.00312406082125351, | ||
| 0.0040978672774037465, | ||
| 0.004431478237276202, | ||
| 0.00409041472149773, | ||
| 0.0031266351819002015, | ||
| 0.0016868781753450394, | ||
| -1.249302315254411e-05, | ||
| -0.001704846339994321, | ||
| -0.003140405829698437, | ||
| -0.0040978672774037465}; | ||
| poly_t sin2_par2[16] = {1.870674613498914e-06, | ||
| 5.292404012785538e-06, | ||
| 7.909829192302831e-06, | ||
| 9.188746390688592e-06, | ||
| 9.313525301268721e-06, | ||
| 7.887020962996302e-06, | ||
| 5.435897856093815e-06, | ||
| 1.8358587462761668e-06, | ||
| -1.870668901922293e-06, | ||
| -5.292404012785538e-06, | ||
| -7.908420336736317e-06, | ||
| -9.320836119343602e-06, | ||
| -9.284396260501616e-06, | ||
| -7.88869635880513e-06, | ||
| -5.262894200243701e-06, | ||
| -1.835864457852788e-06}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be cleaner to have these numbers in an external json file in cms-data?
| hls_met.hwPhi = phi_t(ap_fixed<26, 11>(l1ct::Scales::makeGlbPhi(hls::atan2(met_xy.hwPy, met_xy.hwPx)))); | ||
| #endif | ||
|
|
||
| return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unused return;
| L1METEmu::Sum_Particles(particles_xy, met_xy); | ||
| L1METEmu::pxpy_to_ptphi(met_xy, out_met); | ||
|
|
||
| return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unused return;
| return; | ||
| } | ||
|
|
||
| inline void met_format(l1ct::Sum d, ap_uint<l1gt::Sum::BITWIDTH>& q) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
function inputs should be const refs where possible
|
|
||
| #include "L1Trigger/Phase2L1ParticleFlow/interface/jetmet/L1PFJUMPEmulator.h" | ||
|
|
||
| using namespace l1t; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please avoid using namespace l1t in favor of l1t::func if possible
| iEvent.put(std::move(JUMPCollection)); | ||
| } | ||
|
|
||
| void L1JUMPProducer::CalcJUMP_HLS(l1t::EtSum& metVector, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
function inputs should be const refs where possible
| outMet_Vector.SetEta(0); | ||
| } | ||
|
|
||
| std::vector<l1ct::Jet> L1JUMPProducer::convertEDMToHW(std::vector<l1t::PFJet> edmJets) const { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
function inputs should be const refs where possible
|
|
||
| std::vector<l1ct::Jet> L1JUMPProducer::convertEDMToHW(std::vector<l1t::PFJet> edmJets) const { | ||
| std::vector<l1ct::Jet> hwJets; | ||
| std::for_each(edmJets.begin(), edmJets.end(), [&](l1t::PFJet jet) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better as std::for_each(edmJets.begin(), edmJets.end(), [&](const l1t::PFJet& jet) { but not a big deal
|
please test |
|
+1 Size: This PR adds an extra 32KB to repository The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic: You can see more details here: Comparison SummarySummary:
|
|
Milestone for this pull request has been moved to CMSSW_16_0_X. Please open a backport if it should also go in to CMSSW_15_1_X. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the modifications. Looks good.
|
+l1 |
|
+Upgrade |
|
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. @ftenchini, @mandrenguyen, @sextonkennedy (and backports should be raised in the release meeting by the corresponding L2) |
|
+1 |
|
This PR, and the corresponding cms-data one, are causing 16_0_X IBs to fail: |
PR description:
This PR introduces two new L1 MET calculation algorithms and related implementations:
A new MET emulator using polynomial interpolation has been added. This algorithm was not present in the existing emulator framework and enables smoother, resource-efficient computation of MET using a fitted interpolation curve.
A corresponding extension was made to the existing MET producer to include this new interpolation-based method. This allows users to evaluate and deploy the interpolated MET algorithm directly in CMSSW workflows.
The PR also adds a new algorithm named JUMP (Jet Uncertainty-aware MET Prediction), which uses jet energy resolution information to correct MET in high pileup conditions. Both the producer and the emulator for JUMP were developed from scratch.
DPS Note publicly available on CDS: CMS DP-2025/023
These additions aim to enhance MET performance in challenging pileup scenarios and facilitate hardware emulation and validation of new MET strategies.
PR validation:
All new algorithms have been tested using the FastPUPPI
Used /eos/cms/store/cmst3/group/l1tr/FastPUPPI/14_2_X/fpinputs_140X/v0/TTToSemileptonic_PU200
To support the validation of these algorithms, reference plots prepared by @thesps can be found here:
https://ssummers.web.cern.ch/CMS/TauJetsMET/V44JUMP/ba40f8a/
@thesps