-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Introduce L1TMuonBarrelKalmanParams and Record #34494
Introduce L1TMuonBarrelKalmanParams and Record #34494
Conversation
@panoskatsoulis, CMSSW_12_0_X branch is closed for direct updates. cms-bot is going to move this PR to master branch. |
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-34494/23966
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-34494/23967
|
A new Pull Request was created by @panoskatsoulis (Panos) for master. It involves the following packages:
@malbouis, @yuanchao, @cmsbuild, @rekovic, @tlampen, @ggovi, @pohsun, @francescobrivio, @cecilecaillol, @tvami can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
@rekovic this PR is moved to master (but still under the 12_0_X milestone) since "CMSSW_12_0_X branch is closed for direct updates" as the bot mentions above. |
please test |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-cbd22a/16838/summary.html CMS StaticAnalyzer warnings: There are 4 EventSetupRecord::get warnings. See https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-cbd22a/16838/llvm-analysis/esrget-sa.txt for details. Comparison SummarySummary:
|
@cmsbuild , please test |
Only Francesco can do that :) (and the release managers) |
didn't know, thx |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-cbd22a/17275/summary.html CMS StaticAnalyzer warnings: There are 4 EventSetupRecord::get warnings. See https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-cbd22a/17275/llvm-analysis/esrget-sa.txt for details. Comparison SummaryThe workflows 140.53 have different files in step1_dasquery.log than the ones found in the baseline. You may want to check and retrigger the tests if necessary. You can check it in the "files" directory in the results of the comparisons Summary:
|
unhold |
+alca
|
+l1 |
+1 |
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. @silviodonato, @dpiparo, @qliphy, @perrotta (and backports should be raised in the release meeting by the corresponding L2) |
auto pnodes = ptr->pnodes_[ptr->CONFIG]; | ||
cout << "version : " << ptr->version_ << endl; | ||
cout << "fwVersion : " << hex << pnodes.fwVersion_ << dec << endl; | ||
cout << "LUTsPath : " << pnodes.kalmanLUTsPath_ << endl; |
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.
std::cout is not threadsafe, please use https://twiki.cern.ch/twiki/bin/view/CMSPublic/SWGuideMessageLogger
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, thanks for the notice about this
This module here is an L1T private tester and is not used in any central CMSSW workflow.
The purpose is to read 1 event (only) and print out the info included in the db Rcd
However, if it's required we can update it with MesageLogger in the next iteration of this development, is this fine?
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.
@panoskatsoulis For the next iteration: if it is a private test, it shoud be better moved into the \test area?
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 @perrotta, this is a good point
it is the entire pkg L1TriggerConfig/Utilities
that contains these testing modules
Let me discuss this with the other L1 guys tho, thx
About these warnings, [1] #34494 (comment) |
ok, thanks @panoskatsoulis |
+1 |
Description
This PR introduces new Records and Objects required for implementing a primary "configuration from CondDB" for the L1 Kalman Barrel Emulator. There will be a follow up with the second part of development but this is the bare minimum for testing during the upcoming CRAFT and MWGRs.
The object L1TMuonBarrelKalmanParams holds some (not all yet) of the parameters required by the emulator.
(a static version of these params can be found here [1])
To be discussed during [2]
Validation
This code is validated locally, wrote and read to a local sqlite file
No Backport Needed
[1] https://github.com/cms-sw/cmssw/blob/master/L1Trigger/L1TMuonBarrel/python/simKBmtfDigis_cfi.py
[2] https://indico.cern.ch/event/1060075/#11-introduce-l1tmuonbarrelkalm