-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
uGT fix for muon showers full emulation and for HMT mismatches in the DQM workflow #40773
Conversation
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40773/34199
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-40773/34200
|
A new Pull Request was created by @elfontan (Elisa Fontanesi) for master. It involves the following packages:
@epalencia, @emanueleusai, @cmsbuild, @rekovic, @syuvivida, @pmandrik, @micsucmed, @cecilecaillol, @rvenditti can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
l1t::MuonShower* musOneNominalInTime = new l1t::MuonShower(false, false, false, false, false, false); | ||
l1t::MuonShower* musOneTightInTime = new l1t::MuonShower(false, false, false, false, false, false); | ||
l1t::MuonShower* musOutOfTime0 = new l1t::MuonShower(false, false, false, false, false, false); | ||
l1t::MuonShower* musOutOfTime1 = new l1t::MuonShower(false, false, false, false, false, false); | ||
|
||
musOneNominalInTime->setOneNominalInTime(mu->isOneNominalInTime()); | ||
musOneTightInTime->setOneTightInTime(mu->isOneTightInTime()); | ||
musOutOfTime0->setMusOutOfTime0(mu->musOutOfTime0()); | ||
musOutOfTime1->setMusOutOfTime1(mu->musOutOfTime1()); | ||
|
||
(*m_candL1MuShower).push_back(0, &(*musOneNominalInTime)); | ||
(*m_candL1MuShower).push_back(0, &(*musOneTightInTime)); | ||
(*m_candL1MuShower).push_back(0, &(*musOutOfTime0)); | ||
(*m_candL1MuShower).push_back(0, &(*musOutOfTime1)); | ||
|
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 assume that using a std::unique_ptr
is not possible here because of the .h function interface here?
inline const BXVector<const l1t::MuonShower*>* getCandL1MuShower() const { return m_candL1MuShower; } |
If it is, I think that might be better, because I am concerned that the allocated musOneNominalInTime
, musOneTightInTime
,musOutOfTime0
, & musOutOfTime1
may be leaking memory. They are put into the BXVector m_candL1MuShower
but I don't see anything that explicitly de-allocates this allocated memory, and I don't see anything inside the BXVector code that would do it either.
Later clear
is called here:
cmssw/L1Trigger/L1TGlobal/src/GlobalBoard.cc
Line 1094 in 29ecd54
m_candL1MuShower->clear(); |
but this more or less boils down to calling a std::vector.clear()
I think, which may call destructors, but is not equivalent to delete
on the objects to the best of my knowledge. There is also BX range setting here:
cmssw/L1Trigger/L1TGlobal/src/GlobalBoard.cc
Line 1095 in 29ecd54
m_candL1MuShower->setBXRange(m_bxFirst_, m_bxLast_); |
But I don't think that has an effect if the BX numbers are the same as before, (isn't -2,-1,0,1,2 pretty typical for a BX vector?)
cmssw/DataFormats/L1Trigger/interface/BXVector.icc
Lines 20 to 45 in 08111d0
void BXVector<T>::setBXRange(int bxFirst, int bxLast) { | |
int bxF = bxFirst_; | |
int bxL = bxLast_; | |
if (bxFirst < bxFirst_) { | |
for (int i = 0; i < bxF - bxFirst; i++) { | |
itrs_.insert(itrs_.begin(), itrs_[0]); | |
} | |
} | |
if (bxFirst > bxFirst_) { | |
for (int i = 0; i < bxFirst - bxF; i++) { | |
deleteBX(bxF + i); | |
} | |
} | |
if (bxLast > bxLast_) { | |
for (int i = 0; i < bxLast - bxL; i++) { | |
addBX(); | |
} | |
} | |
if (bxLast < bxLast_) { | |
for (int i = 0; i < bxL - bxLast; i++) { | |
deleteBX(bxL - i); | |
} | |
} | |
bxFirst_ = bxFirst; | |
bxLast_ = bxLast; | |
} |
Perhaps I am missing something though. Is there something I am not seeing that makes sure that the memory allocated for musOneNominalInTime
, musOneTightInTime
,musOutOfTime0
, and musOutOfTime1
is later de-allocated?
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.
Hi @aloeliger , these changes were mostly done by me rather quickly. I didn't try using std::unique_ptr
. This is only one possible fix to this issue. Anybody who is more of an expert on uGT emulator should look into this to see if there are more efficient solutions. I'm also not a huge fan of splitting one object to four objects which creates this problem to begin with. Other uGT objects are just pushed back to the vector just by their iterator.
Please test |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-87b664/30674/summary.html Comparison SummarySummary:
|
@@ -130,6 +130,7 @@ | |||
valGtStage2Digis = simGtStage2Digis.clone( | |||
ExtInputTag = "gtStage2Digis", | |||
MuonInputTag = "gtStage2Digis:Muon", | |||
MuonShowerInputTag = "gmtStage2Digis:MuonShower", |
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.
Once #40822 is confirmed working and merged I think one should use the gt digis here, right?
Thanks so much @dinyar! Sure, I am going to finalize some updates today/tomorrow and I will test your fix together with this PR, and update the MuonShowerInputTag in DQM/L1TMonitor/python/L1TStage2Emulator_cff.py before moving this one to Ready for review. |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40773/34326
|
Pull request #40773 was updated. @aloeliger, @epalencia, @emanueleusai, @cmsbuild, @syuvivida, @pmandrik, @micsucmed, @cecilecaillol, @rvenditti can you please check and sign again. |
please test |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-87b664/30839/summary.html Comparison SummaryThere are some workflows for which there are errors in the baseline: Summary:
|
+1 |
type l1t |
+l1 |
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, @rappoccio (and backports should be raised in the release meeting by the corresponding L2) |
+1 |
PR description:
The purpose of this PR is to address multiple issues at the GT level related to the muon shower emulation. Here two main items are discussed:
Thanks @eyigitba and @aloeliger for the work and the discussion.
The MuonShowerCondition class was broken and was not evaluating at all the condition: this PR contains a first iteration of updates to get reasonable results (with a proper check of the condition for all HMT cases, able to distinguish between Nominal and Tight showers).
Summary of the main issues:
Open items:
Note that L1Ntuple workflow runs the full L1 emulation starting from muonCSCDigis, so the steps in the middle that might be responsible for the discrepancy are: CSC digi unpacker, CSC shower re-emulation, EMTF shower re-emulation.
After a follow up with @kakwok, we realised that the new CSC thresholds for the HMT shower thresholds had not been updated after the deployment online: fix in Update HMT shower thresholds for HMT emulation in CSC #40829. Including this fix, a single event with a mismatch is found (having both a Nominal and Tight shower): 362720:105:115847543.
Further checks with @eyigitba for EMTF and @kakwok for CSC needed.
Github issue in the OSW IB: Muon Shower Emulator Mismatch cms-l1t-offline/cmssw#1070.
Validation:
cmsDriver.py l1Ntuple -s RAW2DIGI --python_filename=data.py -n 1000 --era=Run3 --data --conditions=126X_dataRun3_Prompt_v2 --customise=L1Trigger/L1TNtuples/customiseL1Ntuple.L1NtupleRAWEMU --customise=L1Trigger/Configuration/customiseReEmul.L1TReEmulFromRAW --filein=/store/data/Run2022G/EphemeralHLTPhysics0/RAW/v1/000/362/720/00000/36f350d4-8e8a-4e38-b399-77ad9bf351dc.root --nThreads 8
NOTE: An update of the script to produce test vectors for the uGT firmware-emulator comparison is also provided to rely on the unpacked uGT information as input for the muon shower objects.
PR validation:
Basic tests performed successfully starting from CMSSW_13_0_0_pre4